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

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

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

> M Hickford (2):
>   credential: avoid erasing distinct password
>   credential: erase all matching credentials
>
>  Documentation/git-credential.txt   |   2 +-
>  Documentation/gitcredentials.txt   |   2 +-
>  builtin/credential-cache--daemon.c |  17 +++--
>  builtin/credential-store.c         |  15 +++--
>  credential.c                       |   7 +-
>  credential.h                       |   2 +-
>  t/lib-credential.sh                | 103 +++++++++++++++++++++++++++++
>  7 files changed, 128 insertions(+), 20 deletions(-)

It is helpful to reviewers to describe/summarize, in your own words,
what changed since the previous version, in the cover letter.

The range-diff generated for the versions can serve as a good
supporting material, and it would help you while writing that
summary, but not a substitute for the summary.

Thanks.

> base-commit: d7d8841f67f29e6ecbad85a11805c907d0f00d5d
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1525%2Fhickford%2Ferase-test-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1525/hickford/erase-test-v4
> Pull-Request: https://github.com/git/git/pull/1525
>
> Range-diff vs v3:
>
>  1:  df3c8a15bf8 ! 1:  91d4b04b5e1 credential: avoid erasing distinct password
>      @@ builtin/credential-store.c: static struct lock_file credential_lock;
>        	FILE *fh;
>        	struct strbuf line = STRBUF_INIT;
>       @@ builtin/credential-store.c: static int parse_credential_file(const char *fn,
>      - 
>        	while (strbuf_getline_lf(&line, fh) != EOF) {
>        		if (!credential_from_url_gently(&entry, line.buf, 1) &&
>      --		    entry.username && entry.password &&
>      + 		    entry.username && entry.password &&
>       -		    credential_match(c, &entry)) {
>      -+			entry.username && entry.password &&
>      -+			credential_match(c, &entry, match_password)) {
>      ++		    credential_match(c, &entry, match_password)) {
>        			found_credential = 1;
>        			if (match_cb) {
>        				match_cb(&entry);
>      @@ credential.c: void credential_clear(struct credential *c)
>        {
>        #define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))
>        	return CHECK(protocol) &&
>      --	       CHECK(host) &&
>      --	       CHECK(path) &&
>      + 	       CHECK(host) &&
>      + 	       CHECK(path) &&
>       -	       CHECK(username);
>      -+		CHECK(host) &&
>      -+		CHECK(path) &&
>      -+		CHECK(username) &&
>      -+		(!match_password || CHECK(password));
>      ++	       CHECK(username) &&
>      ++	       (!match_password || CHECK(password));
>        #undef CHECK
>        }
>        
>      @@ t/lib-credential.sh: helper_test_clean() {
>        	reject $1 https example.com user1
>        	reject $1 https example.com user2
>        	reject $1 https example.com user4
>      -+	reject $1 https example.com user5
>      -+	reject $1 https example.com user8
>      ++	reject $1 https example.com user-distinct-pass
>      ++	reject $1 https example.com user-overwrite
>        	reject $1 http path.tld user
>        	reject $1 https timeout.tld user
>        	reject $1 https sso.tld
>      @@ t/lib-credential.sh: helper_test() {
>       +		check approve $HELPER <<-\EOF &&
>       +		protocol=https
>       +		host=example.com
>      -+		username=user8
>      ++		username=user-overwrite
>       +		password=pass1
>       +		EOF
>       +		check approve $HELPER <<-\EOF &&
>       +		protocol=https
>       +		host=example.com
>      -+		username=user8
>      ++		username=user-overwrite
>       +		password=pass2
>       +		EOF
>       +		check fill $HELPER <<-\EOF &&
>       +		protocol=https
>       +		host=example.com
>      -+		username=user8
>      ++		username=user-overwrite
>       +		--
>       +		protocol=https
>       +		host=example.com
>      -+		username=user8
>      ++		username=user-overwrite
>       +		password=pass2
>       +		EOF
>       +		check reject $HELPER <<-\EOF &&
>       +		protocol=https
>       +		host=example.com
>      -+		username=user8
>      ++		username=user-overwrite
>       +		password=pass2
>       +		EOF
>       +		check fill $HELPER <<-\EOF
>       +		protocol=https
>       +		host=example.com
>      -+		username=user8
>      ++		username=user-overwrite
>       +		--
>       +		protocol=https
>       +		host=example.com
>      -+		username=user8
>      ++		username=user-overwrite
>       +		password=askpass-password
>       +		--
>      -+		askpass: Password for '\''https://user8@example.com'\'':
>      ++		askpass: Password for '\''https://user-overwrite@example.com'\'':
>       +		EOF
>       +	'
>       +
>      @@ t/lib-credential.sh: helper_test() {
>       +		check approve $HELPER <<-\EOF &&
>       +		protocol=https
>       +		host=example.com
>      -+		username=user5
>      ++		username=user-distinct-pass
>       +		password=pass1
>       +		EOF
>       +		check reject $HELPER <<-\EOF &&
>       +		protocol=https
>       +		host=example.com
>      -+		username=user5
>      ++		username=user-distinct-pass
>       +		password=pass2
>       +		EOF
>       +		check fill $HELPER <<-\EOF
>       +		protocol=https
>       +		host=example.com
>      -+		username=user5
>      ++		username=user-distinct-pass
>       +		--
>       +		protocol=https
>       +		host=example.com
>      -+		username=user5
>      ++		username=user-distinct-pass
>       +		password=pass1
>       +		EOF
>       +	'
>  2:  e06d80e99a0 ! 2:  42f41b28e6e credential: erase all matching credentials
>      @@ Commit message
>       
>           `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.
>      +    Some helpers (such as credential-store and credential-libsecret) delete
>      +    all matching credentials, others (such as credential-cache) delete at
>      +    most one matching credential.
>       
>           Test that helpers erase all matching credentials. This behaviour is
>           easiest to reason about. Users expect that `echo
>      @@ Commit message
>           "url=https://example.com\nusername=tim" | git credential reject` erase
>           all matching credentials.
>       
>      -    Fix credential-cache and credential-store.
>      +    Fix credential-cache.
>       
>           Signed-off-by: M Hickford <mirth.hickford@gmail.com>
>       
>      @@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE
>        			fprintf(out, "username=%s\n", e->item.username);
>        			fprintf(out, "password=%s\n", e->item.password);
>       
>      - ## builtin/credential-store.c ##
>      -@@ builtin/credential-store.c: static int parse_credential_file(const char *fn,
>      - 			found_credential = 1;
>      - 			if (match_cb) {
>      - 				match_cb(&entry);
>      --				break;
>      - 			}
>      - 		}
>      - 		else if (other_cb)
>      -
>        ## t/lib-credential.sh ##
>       @@ t/lib-credential.sh: 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 https example.com user8
>      + 	reject $1 https example.com user-distinct-pass
>      + 	reject $1 https example.com user-overwrite
>      ++	reject $1 https example.com user-erase1
>      ++	reject $1 https example.com user-erase2
>        	reject $1 http path.tld user
>        	reject $1 https timeout.tld user
>      + 	reject $1 https sso.tld
>       @@ t/lib-credential.sh: helper_test() {
>        		EOF
>        	'
>      @@ t/lib-credential.sh: helper_test() {
>       +		check approve $HELPER <<-\EOF &&
>       +		protocol=https
>       +		host=example.com
>      -+		username=user6
>      ++		username=user-erase1
>       +		password=pass1
>       +		EOF
>       +		check approve $HELPER <<-\EOF &&
>       +		protocol=https
>       +		host=example.com
>      -+		username=user7
>      ++		username=user-erase2
>       +		password=pass1
>       +		EOF
>       +		check reject $HELPER <<-\EOF &&