On 2022-09-19 09:33, Derrick Stolee wrote: > On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote: >> From: Matthew John Cheetham <mjcheetham@xxxxxxxxxxx> > >> In this case we send multiple `wwwauth[n]` properties where `n` is a >> zero-indexed number, reflecting the order the WWW-Authenticate headers >> appeared in the HTTP response. >> @@ -151,6 +151,15 @@ Git understands the following attributes: >> were read (e.g., `url=https://example.com` would behave as if >> `protocol=https` and `host=example.com` had been provided). This >> can help callers avoid parsing URLs themselves. >> + >> +`wwwauth[n]`:: >> + >> + When an HTTP response is received that includes one or more >> + 'WWW-Authenticate' authentication headers, these can be passed to Git >> + (and subsequent credential helpers) with these attributes. >> + Each 'WWW-Authenticate' header value should be passed as a separate >> + attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers >> + appear in the HTTP response. >> + >> Note that specifying a protocol is mandatory and if the URL >> doesn't specify a hostname (e.g., "cert:///path/to/file") the > > This "+" means that this paragraph should be connected to the previous > one, so it seems that you've inserted your new value in the middle of > the `url` key. You'll want to move yours to be after those two connected > paragraphs. Your diff hunk should look like this: > > --- >8 --- > > diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt > index f18673017f..127ae29be3 100644 > --- a/Documentation/git-credential.txt > +++ b/Documentation/git-credential.txt > @@ -160,6 +160,15 @@ empty string. > Components which are missing from the URL (e.g., there is no > username in the example above) will be left unset. > > +`wwwauth[n]`:: > + > + When an HTTP response is received that includes one or more > + 'WWW-Authenticate' authentication headers, these can be passed to Git > + (and subsequent credential helpers) with these attributes. > + Each 'WWW-Authenticate' header value should be passed as a separate > + attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers > + appear in the HTTP response. > + > GIT > --- > Part of the linkgit:git[1] suite > > > --- >8 --- Thanks for catching! >> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf >> index 497b9b9d927..fe118d76f98 100644 >> --- a/t/lib-httpd/apache.conf >> +++ b/t/lib-httpd/apache.conf >> @@ -235,6 +235,19 @@ SSLEngine On >> Require valid-user >> </LocationMatch> >> >> +# Advertise two additional auth methods above "Basic". >> +# Neither of them actually work but serve test cases showing these >> +# additional auth headers are consumed correctly. >> +<Location /auth-wwwauth/> >> + AuthType Basic >> + AuthName "git-auth" >> + AuthUserFile passwd >> + Require valid-user >> + SetEnvIf Authorization "^\S+" authz >> + Header always add WWW-Authenticate "Bearer authority=https://login.example.com"; env=!authz >> + Header always add WWW-Authenticate "FooAuth foo=bar baz=1" env=!authz >> +</Location> >> + > > This is cool that you've figured out how to make our Apache tests > add these headers! Maybe we won't need that extra test helper like > I thought (unless we want to confirm the second request sends the > right information). This will exercise the new header parsing and passing the info to the helper but will indeed not test the response. I feel like a test helper would be beneficial still.. what I've done here doesn't feel 100% clean or complete. >> +test_expect_success 'http auth sends www-auth headers to credential helper' ' >> + write_script git-credential-tee <<-\EOF && >> + cmd=$1 >> + teefile=credential-$cmd >> + if [ -f "$teefile" ]; then > > I think we prefer using "test" over the braces (and linebreak > before then) like this: > > if test -n "$teefile" > then > >> + rm $teefile >> + fi > > Alternatively, you could always run "rm -f $teefile" for > simplicity. I like simple :-) >> + ( >> + while read line; >> + do >> + if [ -z "$line" ]; then >> + exit 0 >> + fi >> + echo "$line" >> $teefile >> + echo $line >> + done >> + ) | git credential-store $cmd > > Since I'm not sure, I'll ask the question: do we need the sub-shell > here, or could we pipe directly off of the "done"? Like this: > > while read line; > do > if [ -z "$line" ]; then > exit 0 > fi > echo "$line" >> $teefile > echo $line > done | git credential-store $cmd That we can.. I will update in next iteration. >> + EOF > > >> + cat >expected-get <<-EOF && >> + protocol=http >> + host=127.0.0.1:5551 >> + wwwauth[0]=Bearer authority=https://login.example.com >> + wwwauth[1]=FooAuth foo=bar baz=1 >> + wwwauth[2]=Basic realm="git-auth" >> + EOF >> + >> + cat >expected-store <<-EOF && >> + protocol=http >> + host=127.0.0.1:5551 >> + username=user@host >> + password=pass@host >> + EOF >> + >> + rm -f .git-credentials && >> + test_config credential.helper tee && >> + set_askpass user@host pass@host && >> + ( >> + PATH="$PWD:$PATH" && >> + git ls-remote "$HTTPD_URL/auth-wwwauth/smart/repo.git" >> + ) && >> + expect_askpass both user@host && >> + test_cmp expected-get credential-get && >> + test_cmp expected-store credential-store > > Elegant check for both calls. > > Thanks, > -Stolee Thanks, Matthew
