Re: [PATCH] credential: new attribute password_expiry_utc

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

"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;
> +		}
> +