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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 6, 2023, 20:32 UTC
Message-ID: <230206.86fsbi5y63.gmgdl@evledraar.gmail.com>
In-Reply-To: <d362f7016d34c4803adf42a88012997c66e0bde8.1675711789.git.gitgitgadget@gmail.com>

On Mon, Feb 06 2023, Matthew John Cheetham via GitGitGadget wrote:

Show 88 quoted lines

> From: Matthew John Cheetham <mjcheetham@outlook.com>
>
> Add a test showing simple anoymous HTTP access to an unprotected
> repository, that results in no credential helper invocations.
> Also add a test demonstrating simple basic authentication with
> simple credential helper support.
>
> 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.
>
> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
> ---
>  t/lib-httpd.sh                 |  1 +
>  t/lib-httpd/apache.conf        |  6 +++
>  t/lib-httpd/nph-custom-auth.sh | 42 +++++++++++++++++
>  t/t5563-simple-http-auth.sh    | 86 ++++++++++++++++++++++++++++++++++
>  4 files changed, 135 insertions(+)
>  create mode 100755 t/lib-httpd/nph-custom-auth.sh
>  create mode 100755 t/t5563-simple-http-auth.sh
>
> 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
>  <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")

Rather than "test -f "$f" & grep ... "$f" I think you can just use only "grep", if the file doesn't exist it'll give you an error.

If you don't want to see that error just pipe it to /dev/null, in case that's what you were trying to avoid with the "check if it exists first".

> +echo 'HTTP/1.1 401 Authorization Required'
> +if test -f "$CHALLENGE_FILE"
> +then
> +	cat "$CHALLENGE_FILE"

Maybe the same here, i.e. just:

	cat "$f" 2>/dev/null

> +test_expect_success 'setup_credential_helper' '
> +	mkdir -p "$TRASH_DIRECTORY/bin" &&

The "$TRASH_DIRECTORY" is already created for you, so don't use "-p", unless something went wrong here..

Show 9 quoted lines

> +	PATH=$PATH:"$TRASH_DIRECTORY/bin" &&
> +	export PATH &&
> +
> +	CREDENTIAL_HELPER="$TRASH_DIRECTORY/bin/git-credential-test-helper" &&
> +	write_script "$CREDENTIAL_HELPER" <<-\EOF
> +	cmd=$1
> +	teefile=$cmd-query.cred
> +	catfile=$cmd-reply.cred
> +	sed -n -e "/^$/q" -e "p" >> $teefile

Style: ">>$f", not ">> $f"

> +	if test "$cmd" = "get"; then

Style: We usually use "\nthen", not "; then".