Re: [PATCH 2/2] credential: erase all matching credentials

>
> "M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: M Hickford <mirth.hickford@gmail.com>
> >
> > 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.

>
> By the way, I do not think your
>
>     cc. Jeff King peff@peff.net cc: Matthew John Cheetham mjcheetham@outlook.com
>
> 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.
>
> > Fixes for credential-cache and credential-store.
> >
> > Signed-off-by: M Hickford <mirth.hickford@gmail.com>
> > ---
> >  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@example.com'\'':
> > +             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))