Re: [PATCH v2 0/2] credential: improvements to erase in helpers

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 14, 2023, 21:56 UTC
Message-ID: <xmqqjzw5wvwk.fsf@gitster.g>
In-Reply-To: <pull.1525.v2.git.git.1686778838.gitgitgadget@gmail.com>

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

> M Hickford (2):
>   credential: avoid erasing distinct password
>   credential: erase all matching credentials

Will queue. Comments by folks with more experience (than me) who have worked on the credential subsystem are very much welcomed.

Thanks.

Show 44 quoted lines

>
>  Documentation/git-credential.txt   |  4 +-
>  Documentation/gitcredentials.txt   |  2 +-
>  builtin/credential-cache--daemon.c | 11 ++++--
>  builtin/credential-store.c         | 21 ++++++-----
>  credential.c                       | 11 +++---
>  credential.h                       |  2 +-
>  t/lib-credential.sh                | 59 ++++++++++++++++++++++++++++++
>  7 files changed, 88 insertions(+), 22 deletions(-)
>
>
> base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1525%2Fhickford%2Ferase-test-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1525/hickford/erase-test-v2
> Pull-Request: https://github.com/git/git/pull/1525
>
> Range-diff vs v1:
>
>  1:  35ee1795bcd = 1:  35ee1795bcd credential: avoid erasing distinct password
>  2:  fcdb579263f ! 2:  9b12f17dc7e credential: erase all matching credentials
>      @@ Metadata
>        ## Commit message ##
>           credential: erase all matching credentials
>       
>      -    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.
>      +    `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.
>       
>      -    Fixes for credential-cache and credential-store.
>      +    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.
>      +
>      +    Fix credential-cache and credential-store.
>       
>           Signed-off-by: M Hickford <mirth.hickford@gmail.com>