Re: [PATCH v4] credential-cache: respect request capabilities
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 8, 2025, 02:05 UTC
- Message-ID
- <xmqqttaaoyaz.fsf@gitster.g>
- In-Reply-To
- <pull.1842.v4.git.1736212760709.gitgitgadget@gmail.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.
Show 73 quoted lines
> 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