| Message ID | 9b12f17dc7ebbab9b1625ad0c6058133b8a53323.1686778838.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | credential: improvements to erase in helpers | expand |
On Wed, Jun 14, 2023 at 09:40:38PM +0000, M Hickford via GitGitGadget wrote: > From: M Hickford <mirth.hickford@gmail.com> > > `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. > > 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. I think this was how it was intended to work all along, but I agree that credential-cache does not behave this way. I think credential-store already works, though. > 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. I read the existing documentation as claiming to delete all such matching credentials, but I guess it is ambiguous (and I did not look, but it is almost a certainty that I wrote the original ;) ). Saying "credentials" in the plural makes it more clear we are getting all of them. But losing "any" makes it less clear to me. Maybe: ...which may erase any stored credentials matching the description. is the best of both? > 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. This one seems like a strict improvement in ambiguity. > 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 > [...] > @@ -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; > + } > } OK, so now instead of looking up a single credential, we loop and cover them all. That makes perfect sense. It is a little funny that we undo the extra parameter to lookup_credential() that was added in the previous commit. I wonder if reversing the order of the patches would make it simpler, but it is probably not a big deal either way. > 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) I think this hunk does nothing. When the op is "erase", we do not pass a match_cb at all, so this break would never trigger (and indeed, it would be disastrous if it did, because we would stop copying entries from the old file to the new, losing everything after the matched entry). And I think if you revert this, your new test still passes. > @@ -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 > + ' The test makes sense. We add two, delete them both, and then check that nothing is left to serve. -Peff
On Wed, 14 Jun 2023 at 23:51, Jeff King <peff@peff.net> wrote: > > On Wed, Jun 14, 2023 at 09:40:38PM +0000, M Hickford via GitGitGadget wrote: > > > From: M Hickford <mirth.hickford@gmail.com> > > > > `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. > > > > 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. > > I think this was how it was intended to work all along, but I agree that > credential-cache does not behave this way. I think credential-store > already works, though. > > > 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. > > I read the existing documentation as claiming to delete all such > matching credentials, but I guess it is ambiguous (and I did not look, > but it is almost a certainty that I wrote the original ;) ). > > Saying "credentials" in the plural makes it more clear we are getting > all of them. But losing "any" makes it less clear to me. Maybe: > > ...which may erase any stored credentials matching the description. > > is the best of both? Will update in patch v3. > > > 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. > > This one seems like a strict improvement in ambiguity. > > > 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 > > [...] > > @@ -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; > > + } > > } > > OK, so now instead of looking up a single credential, we loop and cover > them all. That makes perfect sense. It is a little funny that we undo > the extra parameter to lookup_credential() that was added in the > previous commit. I wonder if reversing the order of the patches would > make it simpler, but it is probably not a big deal either way. > > > 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) > > I think this hunk does nothing. When the op is "erase", we do not pass a > match_cb at all, so this break would never trigger (and indeed, it would > be disastrous if it did, because we would stop copying entries from the > old file to the new, losing everything after the matched entry). > > And I think if you revert this, your new test still passes. You're right. I'll revert in patch v3. > > > @@ -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 > > + ' > > The test makes sense. We add two, delete them both, and then check that > nothing is left to serve. > > -Peff
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))