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

From: Victoria Dye <[email-masked]>
Date: Wed, 14 Dec 2022 15:48:08 -0800

Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <[email-masked]>
> 
> 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.

> 
> Signed-off-by: Matthew John Cheetham <[email-masked]>
> ---
>  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'.

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

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