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

["M Hickford via GitGitGadget" <gitgitgadget@gmail.com>]

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


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 &&

-- 
gitgitgadget