| Message ID | pull.1525.v4.git.git.1686856773.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | credential: improvements to erase in helpers | expand |
"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 &&
On Thu, Jun 15, 2023 at 02:09:15PM -0700, Junio C Hamano wrote: > "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. Yeah, I agree that would have made reviewing much easier. :) That said, I just re-reviewed the patches themselves, and everything now looks good to me. -Peff
Jeff King <peff@peff.net> writes: > On Thu, Jun 15, 2023 at 02:09:15PM -0700, Junio C Hamano wrote: > >> "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. > > Yeah, I agree that would have made reviewing much easier. :) > > That said, I just re-reviewed the patches themselves, and everything now > looks good to me. I missed that you suggested fixing the indentation breakage introduced in the previous round, and the part of the range-diff, which was unexpected to me because I lacked the context, was distracting enough that I missed other changes X-<. But the end result does look good to me, too. Thanks.
Jeff King <peff@peff.net> writes: > That said, I just re-reviewed the patches themselves, and everything now > looks good to me. Thanks for writing and reviewing. Queued.
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 &&