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

[Junio C Hamano <gitster@pobox.com>]

"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))