| Message ID | pull.1443.git.git.1674914650588.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | credential: new attribute password_expiry_utc | expand |
"M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: M Hickford <mirth.hickford@gmail.com> > > If password has expired, credential fill no longer returns early, > so later helpers can generate a fresh credential. This is backwards > compatible -- no change in behaviour with helpers that discard the > expiry attribute. The expiry logic is entirely in the git credential > layer; compatible helpers simply store and return the expiry > attribute verbatim. > > Store new attribute in cache. It is unclear what you are describing in the above. The current behaviour without the patch? The behaviour of the code if this patch gets applied? Write it in such a way that it is clear why the patch is a good idea, not just "this would not hurt because it is backwards compatible". The usual way to do so is to sell your change in this order: - Give background information to help readers understand what you are going to write in the following explanation. - Describe the current behaviour without any change to the code; - Present a situation where the current code results in an undesirable outcome. What exactly happens, what visible effect it has to the user, how the code could do better to help the user? - Propose an updated behaviour that would behave better in the above sample situation presented. Curiously, what you wrote below the "---" line, that will not be part of the log message, looks to be organized better than the above. The first paragraph (except for the "Add ...") prepares the readers, It is still unclear if the second paragraph "when expired" describes what happens with the current code (i.e. highlighting why a change is needed) or what you want to happen with the patch, but the paragraph should first explain the problem in the current behaviour to motivate readers to learn why the updated code would lead to a better world. And follow that with the behaviour of the updated code and its effect (e.g. "without first trying a credential that is stale and see it fail before asking to reauthenticate, such a known-to-be-stale credential gets discarded automatically"). > +`password_expiry_utc`:: > + > + If password is a personal access token or OAuth access token, it may have an expiry date. When getting credentials from a helper, `git credential fill` ignores the password attribute if the expiry date has passed. Storage helpers should store this attribute if possible. Helpers should not implement expiry logic themselves. Represented as Unix time UTC, seconds since 1970. > + A overly long line. Please follow Documentation/CodingGuidelines and Documentation/SubmittingPatches > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > index f3c89831d4a..5cb8a186b45 100644 > --- a/builtin/credential-cache--daemon.c > +++ b/builtin/credential-cache--daemon.c > @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) > if (e) { > fprintf(out, "username=%s\n", e->item.username); > fprintf(out, "password=%s\n", e->item.password); > + if (e->item.password_expiry_utc != 0) { > + fprintf(out, "password_expiry_utc=%ld\n", e->item.password_expiry_utc); > + } Style (multiple issues, check CodingGuidelines): if (e->item.password_expiry_utc) fprintf(out, "... overly long format template ...", e->item.password_expiry_utc); * Using integral value or pointer value as a truth value does not require an explicit comparison with 0; * A single-statement block does not need {} around it; * Overly long line should be folded, with properly indented. > diff --git a/credential.c b/credential.c > index f6389a50684..0a3a9cbf0a2 100644 > --- a/credential.c > +++ b/credential.c > @@ -7,6 +7,7 @@ > #include "prompt.h" > #include "sigchain.h" > #include "urlmatch.h" > +#include <time.h> Don't include system headers directly; often git-compat-util.h already has it, and if not, we need to find the right place to have it in git-compat-util.h file, as there are platforms that are finicky in inclusion order of the header files and definition of feature macros. > @@ -21,6 +22,7 @@ void credential_clear(struct credential *c) > free(c->path); > free(c->username); > free(c->password); > + c->password_expiry_utc = 0; Not a huge deal, but if the rule is "an credential with expiry timestamp that is too old behaves as if it no longer exists or is valid", then a large integer, not zero, may serve as a better sentinel value for "this entry never expires". Instead of having to do if (expiry && expiry < time()) { ... expired ... } you can just do if (expiry < time()) { ... expired ... } and that would be simpler to understand for human readers, too. > @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp) > } else if (!strcmp(key, "path")) { > free(c->path); > c->path = xstrdup(value); > + } else if (!strcmp(key, "password_expiry_utc")) { > + // TODO: ignore if can't parse integer Do not use // comment. /* Our single-liner comment reads like this */ > + c->password_expiry_utc = atoi(value); Don't use atoi(); make sure value is not followed by a non-number, e.g. const char *value = "43q"; printf("%d<%s>\n", atoi(value), value); would give you 43<43q>, but you want to reject and silently ignore such an expiry timestamp. > + // if expiry date has passed, ignore password and expiry fields Ditto, but if you used a large value as sentinel for "never expires" and wrote it like this if (c->password_expiry_utc < time(NULL)) { then it is clear enough that you do not even need such a comment. The expression itself makes it clear what is going on (i.e. the current time comes later than the expiry_utc value on the number line hence it appears on the right to it, clearly showing that it has passed the threshold). > + if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) { > + trace_printf(_("Password has expired.\n")); > + FREE_AND_NULL(c->username); > + FREE_AND_NULL(c->password); > + c->password_expiry_utc = 0; > + } > +
On Sat, Jan 28, 2023 at 9:08 AM M Hickford via GitGitGadget <gitgitgadget@gmail.com> wrote: > If password has expired, credential fill no longer returns early, > so later helpers can generate a fresh credential. This is backwards > compatible -- no change in behaviour with helpers that discard the > expiry attribute. The expiry logic is entirely in the git credential > layer; compatible helpers simply store and return the expiry > attribute verbatim. > > Store new attribute in cache. > > Signed-off-by: M Hickford <mirth.hickford@gmail.com> Just a few comments in addition to those already provided by Junio... > diff --git a/credential.c b/credential.c > @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp) > + // if expiry date has passed, ignore password and expiry fields > + if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) { > + trace_printf(_("Password has expired.\n")); Using `_(...)` marks a string for localization, but doing so is undesirable for debugging messages which are meant for the developer, not the end user (and it creates extra work for translators). No existing[1] trace_printf() calls in the codebase use `_(...)`. [1]: Unfortunately, a couple examples exist in Documentation/MyFirstObjectWalk.txt using `_(...)` but they should be removed. > @@ -269,6 +283,13 @@ void credential_write(const struct credential *c, FILE *fp) > + if (c->password_expiry_utc != 0) { > + int length = snprintf( NULL, 0, "%ld", c->password_expiry_utc); > + char* str = malloc( length + 1 ); Style in this project is `char *str`, not `char* str`. Also, drop spaces around function arguments: char *str = malloc(length + 1); > + snprintf( str, length + 1, "%ld", c->password_expiry_utc ); Same. > + credential_write_item(fp, "password_expiry_utc", str, 0); > + free(str); > + } xstrfmt() from strbuf.h can help simplify this entire block: char *s = xstrfmt("%ld", c->password_expiry_utc); credential_write_item(fp, "password_expiry_utc", str, 0); free(s);
On Sun, 29 Jan 2023 at 20:17, Junio C Hamano <gitster@pobox.com> wrote: > > "M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: M Hickford <mirth.hickford@gmail.com> > > > > If password has expired, credential fill no longer returns early, > > so later helpers can generate a fresh credential. This is backwards > > compatible -- no change in behaviour with helpers that discard the > > expiry attribute. The expiry logic is entirely in the git credential > > layer; compatible helpers simply store and return the expiry > > attribute verbatim. > > > > Store new attribute in cache. > > It is unclear what you are describing in the above. The current > behaviour without the patch? The behaviour of the code if this > patch gets applied? Write it in such a way that it is clear why > the patch is a good idea, not just "this would not hurt because it > is backwards compatible". > > The usual way to do so is to sell your change in this order: > > - Give background information to help readers understand what you > are going to write in the following explanation. > > - Describe the current behaviour without any change to the code; > > - Present a situation where the current code results in an > undesirable outcome. What exactly happens, what visible effect it > has to the user, how the code could do better to help the user? > > - Propose an updated behaviour that would behave better in the > above sample situation presented. > Thanks for the guidance. Writing a better commit message clarified my own thoughts. > Curiously, what you wrote below the "---" line, that will not be > part of the log message, looks to be organized better than the > above. The first paragraph (except for the "Add ...") prepares the > readers, It is still unclear if the second paragraph "when expired" > describes what happens with the current code (i.e. highlighting why > a change is needed) or what you want to happen with the patch, but > the paragraph should first explain the problem in the current > behaviour to motivate readers to learn why the updated code would > lead to a better world. And follow that with the behaviour of the > updated code and its effect (e.g. "without first trying a credential > that is stale and see it fail before asking to reauthenticate, such > a known-to-be-stale credential gets discarded automatically"). > > > > +`password_expiry_utc`:: > > + > > + If password is a personal access token or OAuth access token, it may have an expiry date. When getting credentials from a helper, `git credential fill` ignores the password attribute if the expiry date has passed. Storage helpers should store this attribute if possible. Helpers should not implement expiry logic themselves. Represented as Unix time UTC, seconds since 1970. > > + > > A overly long line. Please follow Documentation/CodingGuidelines > and Documentation/SubmittingPatches > > > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > > index f3c89831d4a..5cb8a186b45 100644 > > --- a/builtin/credential-cache--daemon.c > > +++ b/builtin/credential-cache--daemon.c > > @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) > > if (e) { > > fprintf(out, "username=%s\n", e->item.username); > > fprintf(out, "password=%s\n", e->item.password); > > + if (e->item.password_expiry_utc != 0) { > > + fprintf(out, "password_expiry_utc=%ld\n", e->item.password_expiry_utc); > > + } > > Style (multiple issues, check CodingGuidelines): > > if (e->item.password_expiry_utc) > fprintf(out, "... overly long format template ...", > e->item.password_expiry_utc); > > * Using integral value or pointer value as a truth value does not > require an explicit comparison with 0; > > * A single-statement block does not need {} around it; > > * Overly long line should be folded, with properly indented. > > > diff --git a/credential.c b/credential.c > > index f6389a50684..0a3a9cbf0a2 100644 > > --- a/credential.c > > +++ b/credential.c > > @@ -7,6 +7,7 @@ > > #include "prompt.h" > > #include "sigchain.h" > > #include "urlmatch.h" > > +#include <time.h> > > Don't include system headers directly; often git-compat-util.h > already has it, and if not, we need to find the right place to have > it in git-compat-util.h file, as there are platforms that are > finicky in inclusion order of the header files and definition of > feature macros. > > > @@ -21,6 +22,7 @@ void credential_clear(struct credential *c) > > free(c->path); > > free(c->username); > > free(c->password); > > + c->password_expiry_utc = 0; > > Not a huge deal, but if the rule is "an credential with expiry > timestamp that is too old behaves as if it no longer exists or is > valid", then a large integer, not zero, may serve as a better > sentinel value for "this entry never expires". Instead of having to > do > > if (expiry && expiry < time()) { > ... expired ... > } > > you can just do > > if (expiry < time()) { > ... expired ... > } > > and that would be simpler to understand for human readers, too. > > > @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp) > > } else if (!strcmp(key, "path")) { > > free(c->path); > > c->path = xstrdup(value); > > + } else if (!strcmp(key, "password_expiry_utc")) { > > + // TODO: ignore if can't parse integer > > Do not use // comment. /* Our single-liner comment reads like this */ > > > + c->password_expiry_utc = atoi(value); > > Don't use atoi(); make sure value is not followed by a non-number, > e.g. > > const char *value = "43q"; > printf("%d<%s>\n", atoi(value), value); > > would give you 43<43q>, but you want to reject and silently ignore > such an expiry timestamp. > > > + // if expiry date has passed, ignore password and expiry fields > > Ditto, but if you used a large value as sentinel for "never expires" > and wrote it like this > > if (c->password_expiry_utc < time(NULL)) { > > then it is clear enough that you do not even need such a comment. > The expression itself makes it clear what is going on (i.e. the > current time comes later than the expiry_utc value on the number > line hence it appears on the right to it, clearly showing that it > has passed the threshold). > > > + if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) { > > + trace_printf(_("Password has expired.\n")); > > + FREE_AND_NULL(c->username); > > + FREE_AND_NULL(c->password); > > + c->password_expiry_utc = 0; > > + } > > +
M Hickford <mirth.hickford@gmail.com> writes: > Thanks for the guidance. Writing a better commit message clarified my > own thoughts. It is great to hear that somebody agreeing with me on this point. We try to write in a way that would make it clear to readers of "git log", but I often notice that trying to explain the changes to the code clearly does force me to realize better ways to write the changes.
Thanks Eric for the review On Mon, 30 Jan 2023 at 00:59, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Sat, Jan 28, 2023 at 9:08 AM M Hickford via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > If password has expired, credential fill no longer returns early, > > so later helpers can generate a fresh credential. This is backwards > > compatible -- no change in behaviour with helpers that discard the > > expiry attribute. The expiry logic is entirely in the git credential > > layer; compatible helpers simply store and return the expiry > > attribute verbatim. > > > > Store new attribute in cache. > > > > Signed-off-by: M Hickford <mirth.hickford@gmail.com> > > Just a few comments in addition to those already provided by Junio... > > > diff --git a/credential.c b/credential.c > > @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp) > > + // if expiry date has passed, ignore password and expiry fields > > + if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) { > > + trace_printf(_("Password has expired.\n")); > > Using `_(...)` marks a string for localization, but doing so is > undesirable for debugging messages which are meant for the developer, > not the end user (and it creates extra work for translators). No > existing[1] trace_printf() calls in the codebase use `_(...)`. Done in patch v3. > > [1]: Unfortunately, a couple examples exist in > Documentation/MyFirstObjectWalk.txt using `_(...)` but they should be > removed. > > > @@ -269,6 +283,13 @@ void credential_write(const struct credential *c, FILE *fp) > > + if (c->password_expiry_utc != 0) { > > + int length = snprintf( NULL, 0, "%ld", c->password_expiry_utc); > > + char* str = malloc( length + 1 ); > > Style in this project is `char *str`, not `char* str`. Also, drop > spaces around function arguments: > > char *str = malloc(length + 1); > > > + snprintf( str, length + 1, "%ld", c->password_expiry_utc ); > > Same. > > > + credential_write_item(fp, "password_expiry_utc", str, 0); > > + free(str); > > + } > > xstrfmt() from strbuf.h can help simplify this entire block: > > char *s = xstrfmt("%ld", c->password_expiry_utc); > credential_write_item(fp, "password_expiry_utc", str, 0); > free(s); Neat. Done in patch v3.
diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt index ac2818b9f66..15ace648bdd 100644 --- a/Documentation/git-credential.txt +++ b/Documentation/git-credential.txt @@ -144,6 +144,10 @@ Git understands the following attributes: The credential's password, if we are asking it to be stored. +`password_expiry_utc`:: + + If password is a personal access token or OAuth access token, it may have an expiry date. When getting credentials from a helper, `git credential fill` ignores the password attribute if the expiry date has passed. Storage helpers should store this attribute if possible. Helpers should not implement expiry logic themselves. Represented as Unix time UTC, seconds since 1970. + `url`:: When this special attribute is read by `git credential`, the diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c index f3c89831d4a..5cb8a186b45 100644 --- a/builtin/credential-cache--daemon.c +++ b/builtin/credential-cache--daemon.c @@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out) if (e) { fprintf(out, "username=%s\n", e->item.username); fprintf(out, "password=%s\n", e->item.password); + if (e->item.password_expiry_utc != 0) { + fprintf(out, "password_expiry_utc=%ld\n", e->item.password_expiry_utc); + } } } else if (!strcmp(action.buf, "exit")) { diff --git a/credential.c b/credential.c index f6389a50684..0a3a9cbf0a2 100644 --- a/credential.c +++ b/credential.c @@ -7,6 +7,7 @@ #include "prompt.h" #include "sigchain.h" #include "urlmatch.h" +#include <time.h> void credential_init(struct credential *c) { @@ -21,6 +22,7 @@ void credential_clear(struct credential *c) free(c->path); free(c->username); free(c->password); + c->password_expiry_utc = 0; string_list_clear(&c->helpers, 0); credential_init(c); @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp) } else if (!strcmp(key, "path")) { free(c->path); c->path = xstrdup(value); + } else if (!strcmp(key, "password_expiry_utc")) { + // TODO: ignore if can't parse integer + c->password_expiry_utc = atoi(value); } else if (!strcmp(key, "url")) { credential_from_url(c, value); } else if (!strcmp(key, "quit")) { c->quit = !!git_config_bool("quit", value); } + + // if expiry date has passed, ignore password and expiry fields + if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) { + trace_printf(_("Password has expired.\n")); + FREE_AND_NULL(c->username); + FREE_AND_NULL(c->password); + c->password_expiry_utc = 0; + } + /* * Ignore other lines; we don't know what they mean, but * this future-proofs us when later versions of git do @@ -269,6 +283,13 @@ void credential_write(const struct credential *c, FILE *fp) credential_write_item(fp, "path", c->path, 0); credential_write_item(fp, "username", c->username, 0); credential_write_item(fp, "password", c->password, 0); + if (c->password_expiry_utc != 0) { + int length = snprintf( NULL, 0, "%ld", c->password_expiry_utc); + char* str = malloc( length + 1 ); + snprintf( str, length + 1, "%ld", c->password_expiry_utc ); + credential_write_item(fp, "password_expiry_utc", str, 0); + free(str); + } } static int run_credential_helper(struct credential *c, diff --git a/credential.h b/credential.h index f430e77fea4..e10f7c2b313 100644 --- a/credential.h +++ b/credential.h @@ -126,6 +126,7 @@ struct credential { char *protocol; char *host; char *path; + time_t password_expiry_utc; }; #define CREDENTIAL_INIT { \