On Wed, 14 Jun 2023 at 17:00, Junio C Hamano <gitster@xxxxxxxxx> wrote: > > "M Hickford via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > > > From: M Hickford <mirth.hickford@xxxxxxxxx> > > > > Users expect that > > `echo "url=https://example.com"; | git credential reject` or > > `echo "url=https://example.com\nusername=tim"; | git credential reject` > > erase all matching credentials. > > Perhaps that is a sensible expectation. It is unclear from the > above description what happens instead without the "fix", though. `credential reject` sends the erase action to each helper, but the exact behaviour of erase isn't specified in documentation or tests. Some helpers (such as credential-libsecret) delete all matching credentials, others (such as credential-cache and credential-store) delete at most one matching credential. Test that helpers erase all matching credentials. This behaviour is easiest to reason about. Users expect that `echo "url=https://example.com"; | git credential reject` or `echo "url=https://example.com\nusername=tim"; | git credential reject` erase all matching credentials. I'll update the commit message in patch v2. > > By the way, I do not think your > > cc. Jeff King peff@xxxxxxxx cc: Matthew John Cheetham mjcheetham@xxxxxxxxxxx > > in 0/2 is doing anything; I've manually added them to Cc: of this > message and left everything in the patch below, even though I am not > commenting on anything there myself, to give them easier reference. Thanks Junio. I'll check the GitGitGadget documentation. > > Thanks. > > > Fixes for credential-cache and credential-store. > > > > Signed-off-by: M Hickford <mirth.hickford@xxxxxxxxx> > > --- > > Documentation/git-credential.txt | 4 ++-- > > Documentation/gitcredentials.txt | 2 +- > > builtin/credential-cache--daemon.c | 15 ++++++++------ > > builtin/credential-store.c | 3 ++- > > t/lib-credential.sh | 33 ++++++++++++++++++++++++++++++ > > 5 files changed, 47 insertions(+), 10 deletions(-) > > > > diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt > > index 0e6d9e85ec7..04bfb918de6 100644 > > --- a/Documentation/git-credential.txt > > +++ b/Documentation/git-credential.txt > > @@ -38,8 +38,8 @@ to any configured credential helpers, which may store the credential > > for later use. > > > > If the action is `reject`, git-credential will send the description to > > -any configured credential helpers, which may erase any stored > > -credential matching the description. > > +any configured credential helpers, which may erase stored credentials > > +matching the description. > > > > If the action is `approve` or `reject`, no output should be emitted. > > > > diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt > > index 100f045bb1a..65d652dc40e 100644 > > --- a/Documentation/gitcredentials.txt > > +++ b/Documentation/gitcredentials.txt > > @@ -260,7 +260,7 @@ appended to its command line, which is one of: > > > > `erase`:: > > > > - Remove a matching credential, if any, from the helper's storage. > > + Remove matching credentials, if any, from the helper's storage. > > > > The details of the credential will be provided on the helper's stdin > > stream. The exact format is the same as the input/output format of the > > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > > index 82f376d3351..5e3a766e42d 100644 > > --- a/builtin/credential-cache--daemon.c > > +++ b/builtin/credential-cache--daemon.c > > @@ -33,12 +33,12 @@ static void cache_credential(struct credential *c, int timeout) > > e->expiration = time(NULL) + timeout; > > } > > > > -static struct credential_cache_entry *lookup_credential(const struct credential *c, int match_password) > > +static struct credential_cache_entry *lookup_credential(const struct credential *c) > > { > > int i; > > for (i = 0; i < entries_nr; i++) { > > struct credential *e = &entries[i].item; > > - if (credential_match(c, e, match_password)) > > + if (credential_match(c, e, 0)) > > return &entries[i]; > > } > > return NULL; > > @@ -48,9 +48,12 @@ static void remove_credential(const struct credential *c) > > { > > struct credential_cache_entry *e; > > > > - e = lookup_credential(c, c->password != NULL); > > - if (e) > > - e->expiration = 0; > > + int i; > > + for (i = 0; i < entries_nr; i++) { > > + e = &entries[i]; > > + if (credential_match(c, &e->item, c->password != NULL)) > > + e->expiration = 0; > > + } > > } > > > > static timestamp_t check_expirations(void) > > @@ -127,7 +130,7 @@ static void serve_one_client(FILE *in, FILE *out) > > if (read_request(in, &c, &action, &timeout) < 0) > > /* ignore error */ ; > > else if (!strcmp(action.buf, "get")) { > > - struct credential_cache_entry *e = lookup_credential(&c, 0); > > + struct credential_cache_entry *e = lookup_credential(&c); > > if (e) { > > fprintf(out, "username=%s\n", e->item.username); > > fprintf(out, "password=%s\n", e->item.password); > > diff --git a/builtin/credential-store.c b/builtin/credential-store.c > > index e0ae028b1c3..85b147e460f 100644 > > --- a/builtin/credential-store.c > > +++ b/builtin/credential-store.c > > @@ -36,7 +36,8 @@ static int parse_credential_file(const char *fn, > > found_credential = 1; > > if (match_cb) { > > match_cb(&entry); > > - break; > > + if (strcmp(op, "erase")) > > + break; > > } > > } > > else if (other_cb) > > diff --git a/t/lib-credential.sh b/t/lib-credential.sh > > index f7e4e29c5e1..3f4100b6ce2 100644 > > --- a/t/lib-credential.sh > > +++ b/t/lib-credential.sh > > @@ -45,6 +45,8 @@ helper_test_clean() { > > reject $1 https example.com user2 > > reject $1 https example.com user4 > > reject $1 https example.com user5 > > + reject $1 https example.com user6 > > + reject $1 https example.com user7 > > reject $1 http path.tld user > > reject $1 https timeout.tld user > > reject $1 https sso.tld > > @@ -298,6 +300,37 @@ helper_test() { > > EOF > > ' > > > > + test_expect_success "helper ($HELPER) erases all matching credentials" ' > > + check approve $HELPER <<-\EOF && > > + protocol=https > > + host=example.com > > + username=user6 > > + password=pass1 > > + EOF > > + check approve $HELPER <<-\EOF && > > + protocol=https > > + host=example.com > > + username=user7 > > + password=pass1 > > + EOF > > + check reject $HELPER <<-\EOF && > > + protocol=https > > + host=example.com > > + EOF > > + check fill $HELPER <<-\EOF > > + protocol=https > > + host=example.com > > + -- > > + protocol=https > > + host=example.com > > + username=askpass-username > > + password=askpass-password > > + -- > > + askpass: Username for '\''https://example.com'\'': > > + askpass: Password for '\''https://askpass-username@xxxxxxxxxxx'\'': > > + EOF > > + ' > > + > > : ${GIT_TEST_LONG_CRED_BUFFER:=1024} > > # 23 bytes accounts for "wwwauth[]=basic realm=" plus NUL > > LONG_VALUE_LEN=$((GIT_TEST_LONG_CRED_BUFFER - 23))
