Re: [PATCH v4 8/8] t5556: add HTTP authentication tests

From: Victoria Dye <vdye@github.com>
Date: Dec 14, 2022, 23:48 UTC
Message-ID: <1dc44716-2550-47de-e666-9972b102905d@github.com>
In-Reply-To: <8ecf63835229676677e3f7e33f634eb5d3a568b7.1670880984.git.gitgitgadget@gmail.com>

Matthew John Cheetham via GitGitGadget wrote:

Show 5 quoted lines

> From: Matthew John Cheetham <mjcheetham@outlook.com>
> 
> Add a series of tests to exercise the HTTP authentication header parsing
> and the interop with credential helpers. Credential helpers will receive
> WWW-Authenticate information in credential requests.

A general comment about this series - the way you have the patches organized means that the "feature" content you're trying to integrate (the first two patches) is contextually separated from these tests. For people that learn/understand code via examples in tests, this makes it really difficult to understand what's going on. To avoid that, I think you could rearrange the patches pretty easily:

1. test-http-server: add stub HTTP server test helper (prev. patch 3)
  - t5556 could be introduced here with the basic "anonymous" test in patch
    6, but marked 'test_expect_failure'.
2. test-http-server: add HTTP error response function (prev. patch 4)
3. test-http-server: add HTTP request parsing (prev. patch 5)
4. test-http-server: pass Git requests to http-backend (prev. patch 6)
5. test-http-server: add simple authentication (prev. patch 7)
6. http: read HTTP WWW-Authenticate response headers (prev. patch 1)
7. credential: add WWW-Authenticate header to cred requests (prev patch 2)
  - Some/all of the tests from the current patch (patch 8) could be squashed
    into this one so that the tests exist directly alongside the new
    functionality they're testing.

Show 13 quoted lines

> 
> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
> ---
>  t/helper/test-credential-helper-replay.sh |  14 +++
>  t/t5556-http-auth.sh                      | 120 +++++++++++++++++++++-
>  2 files changed, 133 insertions(+), 1 deletion(-)
>  create mode 100755 t/helper/test-credential-helper-replay.sh
> 
> diff --git a/t/helper/test-credential-helper-replay.sh b/t/helper/test-credential-helper-replay.sh
> new file mode 100755
> index 00000000000..03e5e63dad6
> --- /dev/null
> +++ b/t/helper/test-credential-helper-replay.sh

I'm not sure a 't/helper' file is the right place for this - it's a pretty simple shell script, but it defines a lot of information (namely 'teefile', 'catfile') that is otherwise unexplained in 't5556'.

What about something like 'lib-rebase.sh' and its 'set_fake_editor()'? You could create a similar test lib ('lib-credential-helper.sh') and wrapper function (' that writes out a custom credential helper. Something like 'set_fake_credential_helper()' could also take 'teefile' and 'catfile' as arguments, making their names more transparent to 't5556'.

Show 25 quoted lines

> @@ -0,0 +1,14 @@
> +cmd=$1
> +teefile=$cmd-actual.cred
> +catfile=$cmd-response.cred
> +rm -f $teefile
> +while read line;
> +do
> +	if test -z "$line"; then
> +		break;
> +	fi
> +	echo "$line" >> $teefile
> +done
> +if test "$cmd" = "get"; then
> +	cat $catfile
> +fi
> diff --git a/t/t5556-http-auth.sh b/t/t5556-http-auth.sh
> index 78da151f122..541fa32bd77 100755
> --- a/t/t5556-http-auth.sh
> +++ b/t/t5556-http-auth.sh
> @@ -26,6 +26,8 @@ PID_FILE="$(pwd)"/pid-file.pid
>  SERVER_LOG="$(pwd)"/OUT.server.log
>  
>  PATH="$GIT_BUILD_DIR/t/helper/:$PATH" && export PATH
> +CREDENTIAL_HELPER="$GIT_BUILD_DIR/t/helper/test-credential-helper-replay.sh" \
> +	&& export CREDENTIAL_HELPER

I see - this is how you connect the "test" credential helper to the HTTP server and header parsing (as implemented in patches 1 & 2), so that the results can be compared for correctness.

nit: you can just 'export CREDENTIAL_HELPER="..."', rather than breaking it into two lines. You also shouldn't need to 'export' at all - the value will be set in the context of the test.

Show 18 quoted lines

>  
>  test_expect_success 'setup repos' '
>  	test_create_repo "$REPO_DIR" &&
> @@ -91,7 +93,8 @@ start_http_server () {
>  
>  per_test_cleanup () {
>  	stop_http_server &&
> -	rm -f OUT.*
> +	rm -f OUT.* &&
> +	rm -f *.cred
>  }
>  
>  test_expect_success 'http auth anonymous no challenge' '
> @@ -102,4 +105,119 @@ test_expect_success 'http auth anonymous no challenge' '
>  	git ls-remote $ORIGIN_URL
>  '
>  
> +test_expect_success 'http auth www-auth headers to credential helper basic valid' '

...

> +test_expect_success 'http auth www-auth headers to credential helper custom schemes' '

...

> +test_expect_success 'http auth www-auth headers to credential helper invalid' '

These tests all look good. That said, is there any way to test more bizarre/edge cases (headers too long to fit on one line, headers that end with a long string of whitespace, etc.)?