| Message ID | 8ecf63835229676677e3f7e33f634eb5d3a568b7.1670880984.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enhance credential helper protocol to include auth headers | expand |
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. > > 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'. > @@ -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.)?
Victoria Dye <vdye@github.com> writes: > 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: > ... Thanks for a thorough review of the entire series, with concrete suggestions for improvements with encouragements sprinkled in. Very much appreciated.
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
On 2022-12-14 16:21, Junio C Hamano wrote: > Victoria Dye <vdye@github.com> writes: > >> 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: >> ... > > Thanks for a thorough review of the entire series, with concrete > suggestions for improvements with encouragements sprinkled in. > > Very much appreciated. > Yes! Thank you Victoria for the detailed and thorough review. I also too very much appreciate it :-) Thanks, Matthew
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 @@ -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 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_when_finished "per_test_cleanup" && + # base64("alice:secret-passwd") + USERPASS64=YWxpY2U6c2VjcmV0LXBhc3N3ZA== && + export USERPASS64 && + + start_http_server \ + --auth=basic:realm=\"example.com\" \ + --auth-token=basic:$USERPASS64 && + + cat >get-expected.cred <<-EOF && + protocol=http + host=$HOST_PORT + wwwauth[]=basic realm="example.com" + EOF + + cat >store-expected.cred <<-EOF && + protocol=http + host=$HOST_PORT + username=alice + password=secret-passwd + EOF + + cat >get-response.cred <<-EOF && + protocol=http + host=$HOST_PORT + username=alice + password=secret-passwd + EOF + + git -c credential.helper="$CREDENTIAL_HELPER" ls-remote $ORIGIN_URL && + + test_cmp get-expected.cred get-actual.cred && + test_cmp store-expected.cred store-actual.cred +' + +test_expect_success 'http auth www-auth headers to credential helper custom schemes' ' + test_when_finished "per_test_cleanup" && + # base64("alice:secret-passwd") + USERPASS64=YWxpY2U6c2VjcmV0LXBhc3N3ZA== && + export USERPASS64 && + + start_http_server \ + --auth=foobar:alg=test\ widget=1 \ + --auth=bearer:authority=\"id.example.com\"\ q=1\ p=0 \ + --auth=basic:realm=\"example.com\" \ + --auth-token=basic:$USERPASS64 && + + cat >get-expected.cred <<-EOF && + protocol=http + host=$HOST_PORT + wwwauth[]=foobar alg=test widget=1 + wwwauth[]=bearer authority="id.example.com" q=1 p=0 + wwwauth[]=basic realm="example.com" + EOF + + cat >store-expected.cred <<-EOF && + protocol=http + host=$HOST_PORT + username=alice + password=secret-passwd + EOF + + cat >get-response.cred <<-EOF && + protocol=http + host=$HOST_PORT + username=alice + password=secret-passwd + EOF + + git -c credential.helper="$CREDENTIAL_HELPER" ls-remote $ORIGIN_URL && + + test_cmp get-expected.cred get-actual.cred && + test_cmp store-expected.cred store-actual.cred +' + +test_expect_success 'http auth www-auth headers to credential helper invalid' ' + test_when_finished "per_test_cleanup" && + # base64("alice:secret-passwd") + USERPASS64=YWxpY2U6c2VjcmV0LXBhc3N3ZA== && + export USERPASS64 && + start_http_server \ + --auth=bearer:authority=\"id.example.com\"\ q=1\ p=0 \ + --auth=basic:realm=\"example.com\" \ + --auth-token=basic:$USERPASS64 && + + cat >get-expected.cred <<-EOF && + protocol=http + host=$HOST_PORT + wwwauth[]=bearer authority="id.example.com" q=1 p=0 + wwwauth[]=basic realm="example.com" + EOF + + cat >erase-expected.cred <<-EOF && + protocol=http + host=$HOST_PORT + username=alice + password=invalid-passwd + wwwauth[]=bearer authority="id.example.com" q=1 p=0 + wwwauth[]=basic realm="example.com" + EOF + + cat >get-response.cred <<-EOF && + protocol=http + host=$HOST_PORT + username=alice + password=invalid-passwd + EOF + + test_must_fail git -c credential.helper="$CREDENTIAL_HELPER" ls-remote $ORIGIN_URL && + + test_cmp get-expected.cred get-actual.cred && + test_cmp erase-expected.cred erase-actual.cred +' + test_done