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

[Matthew John Cheetham <mjcheetham@outlook.com>]

On 2022-12-14 15:48, Victoria Dye wrote:

> Matthew John Cheetham via GitGitGadget wrote:
>> 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.


I think that order make sense - I'll rearrange for my next iteration.
Thanks!

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

The `lib-rebase.sh` script sets the fake editor by setting an environment
variable (from what I can see). Credential helpers can only be set via config
or command-line arg. Would it be easier to move writing of the test credential
helper script to the t5556 test script setup?

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

I tried this originally, but got errors from one of the environments in CI that
this was not portable.

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

Thanks,
Matthew