On Mon, 6 Jan 2025 at 22:32, brian m. carlson
<sandals@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On 2025-01-06 at 19:52:11, M Hickford via GitGitGadget wrote:
> > From: M Hickford <mirth.hickford@xxxxxxxxx>
> >
> > Previously, credential-cache responded with capability[]=authtype
> > regardless of request.
>
> That's the correct behaviour.
>
> > The capabilities in a credential helper response should be a subset of
> > the capabilities in the request.
>
> No, it should not. Otherwise, it's impossible for Git to know whether
> the helper does or does not support the capability. We rely on that
> information to correctly pass data back when saving data.
>
> > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> > index bc22f5c6d24..692216cf83c 100644
> > --- a/builtin/credential-cache--daemon.c
> > +++ b/builtin/credential-cache--daemon.c
> > @@ -134,17 +134,16 @@ static void serve_one_client(FILE *in, FILE *out)
> > else if (!strcmp(action.buf, "get")) {
> > struct credential_cache_entry *e = lookup_credential(&c);
> > if (e) {
> > - e->item.capa_authtype.request_initial = 1;
> > - e->item.capa_authtype.request_helper = 1;
> > -
> > - fprintf(out, "capability[]=authtype\n");
> > + if (credential_has_capability(&c.capa_authtype, CREDENTIAL_OP_RESPONSE)) {
> > + fprintf(out, "capability[]=authtype\n");
> > + }
>
> This part is not correct.
Thanks for the review. I'll revert this part and amend the commit message.
>
> > if (e->item.username)
> > 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)
>
> This part may very well be correct.
>
> > 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..324ecc792d5 100644
> > --- a/t/lib-credential.sh
> > +++ b/t/lib-credential.sh
> > @@ -566,6 +566,21 @@ helper_test_authtype() {
> > EOF
> > '
> >
> > + test_expect_success "helper ($HELPER) get authtype 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@xxxxxxxxxxxxxxx'\'':
> > + EOF
> > + '
> > +
> > test_expect_success "helper ($HELPER) stores authtype and credential with username" '
> > check approve $HELPER <<-\EOF
> > capability[]=authtype
> >
> > base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f
> > --
> > gitgitgadget
>
> --
> brian m. carlson (they/them or he/him)
> Toronto, Ontario, CA
