On 2022-12-14 15:48, Victoria Dye wrote:
> Matthew John Cheetham via GitGitGadget wrote:
>> From: Matthew John Cheetham <mjcheetham@xxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxx>
>> ---
>> 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
