Re: [PATCH v4] credential-cache: respect request capabilities

[Junio C Hamano <gitster@pobox.com>]

"M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: M Hickford <mirth.hickford@gmail.com>
>
> Previously, credential-cache populated authtype regardless of request.

OK, that may be a correct statement of the fact, but it does not
tell readers any of the following:

 - If it is a bad thing to populate authtype regardless of request,
   and if so why?

 - What is the (negative) consequence of doing so, if any?  What
   breaks because it populates authtype regardless of request?

 - What is the remedy?  Instead of unconditionally populating the
   authtype, how does the new code decide when to populate it and
   with what value?

 - Can there be downsides of fixing this?  Are there use cases where
   this unconditional population of authtype was relied upon?

 - Where did the bug come from and what is its fix?  We used to look
   at OP_HELPER to decide when to emit authtype, but the updated
   code checks OP_RESPONSE, which readers can see in the patch.

   It would be nice if the proposed log message helped them by
   briefly explaining their differences, for example.

which would help future "git log" readers what this fix was about.

Will queue for now, but the log message would want to be a bit more
helpful to the readers.

Thanks.

> Signed-off-by: M Hickford <mirth.hickford@gmail.com>
> ---
>     credential-cache: respect request capabilities
>     
>     CC: sandals@crustytoothpaste.net
>     
>     Patch v4 fixes test
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1842%2Fhickford%2Fcache-capability-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1842/hickford/cache-capability-v4
> Pull-Request: https://github.com/gitgitgadget/git/pull/1842
>
> Range-diff vs v3:
>
>  1:  e9851c5c4ac ! 1:  23942f9fa47 credential-cache: respect request capabilities
>      @@ t/lib-credential.sh: helper_test_authtype() {
>       +		protocol=https
>       +		host=git.example.com
>       +		--
>      -+		capability[]=authtype
>       +		protocol=https
>       +		host=git.example.com
>       +		username=askpass-username
>
>
>  builtin/credential-cache--daemon.c |  4 ++--
>  t/lib-credential.sh                | 15 +++++++++++++++
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> index bc22f5c6d24..e707618e743 100644
> --- a/builtin/credential-cache--daemon.c
> +++ b/builtin/credential-cache--daemon.c
> @@ -142,9 +142,9 @@ static void serve_one_client(FILE *in, FILE *out)
>  				fprintf(out, "username=%s\n", e->item.username);
>  			if (e->item.password)
>  				fprintf(out, "password=%s\n", e->item.password);
> -			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.authtype)
> +			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.authtype)
>  				fprintf(out, "authtype=%s\n", e->item.authtype);
> -			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_HELPER) && e->item.credential)
> +			if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE) && e->item.credential)
>  				fprintf(out, "credential=%s\n", e->item.credential);
>  			if (e->item.password_expiry_utc != TIME_MAX)
>  				fprintf(out, "password_expiry_utc=%"PRItime"\n",
> diff --git a/t/lib-credential.sh b/t/lib-credential.sh
> index 58b9c740605..cc6bf9aa5f3 100644
> --- a/t/lib-credential.sh
> +++ b/t/lib-credential.sh
> @@ -566,6 +566,21 @@ helper_test_authtype() {
>  		EOF
>  	'
>  
> +	test_expect_success "helper ($HELPER) gets authtype and credential only if request has authtype capability" '
> +		check fill $HELPER <<-\EOF
> +		protocol=https
> +		host=git.example.com
> +		--
> +		protocol=https
> +		host=git.example.com
> +		username=askpass-username
> +		password=askpass-password
> +		--
> +		askpass: Username for '\''https://git.example.com'\'':
> +		askpass: Password for '\''https://askpass-username@git.example.com'\'':
> +		EOF
> +	'
> +
>  	test_expect_success "helper ($HELPER) stores authtype and credential with username" '
>  		check approve $HELPER <<-\EOF
>  		capability[]=authtype
>
> base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f