Re: [PATCH 12/13] credentials: add "store" helper

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 29, 2011, 18:19 UTC
Message-ID: <7vsjl6ssf9.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20111124110756.GJ8417@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 20 quoted lines

> +static void parse_credential_file(const char *fn,
> +				  struct credential *c,
> +				  void (*match_cb)(struct credential *),
> +				  void (*other_cb)(struct strbuf *))
> +{
> +	FILE *fh;
> +	struct strbuf line = STRBUF_INIT;
> +	struct credential entry = CREDENTIAL_INIT;
> +
> +	fh = fopen(fn, "r");
> +	if (!fh) {
> +		if (errno != ENOENT)
> +			die_errno("unable to open %s", fn);
> +		return;
> +	}
> +
> +	while (strbuf_getline(&line, fh, '\n') != EOF) {
> +		credential_from_url(&entry, line.buf);
> +		if (entry.username && entry.password &&
> +		    credential_match(c, &entry)) {

This looks curious; isn't checking .username and .password part of the responsibility of credential_match()? And even if entry lacks password (which won't happen in the context of this program, given the implementation of store_credential() below) shouldn't it still be considered a match?

> +			if (match_cb) {
> +				match_cb(&entry);
> +				break;

Multiple matches not allowed, which sounds fine.

Show 27 quoted lines

> +			}
> +		}
> +		else if (other_cb)
> +			other_cb(&line);
> +	}
> +
> +	credential_clear(&entry);
> +	strbuf_release(&line);
> +	fclose(fh);
> +}
> +
> +static void print_entry(struct credential *c)
> +{
> +	printf("username=%s\n", c->username);
> +	printf("password=%s\n", c->password);
> +}
> +
> +static void print_line(struct strbuf *buf)
> +{
> +	strbuf_addch(buf, '\n');
> +	write_or_die(credential_lock.fd, buf->buf, buf->len);
> +}
> +
> +static void rewrite_credential_file(const char *fn, struct credential *c,
> +				    struct strbuf *extra)
> +{
> +	umask(077);

Curious placement of umask(). I would expect a function that has its own call to umask() restore it before it returns, and a stand-alone program whose sole purpose is to work with a private file, setting a tight umask upfront at the beginning of main() may be easier to understand.

Show 5 quoted lines

> +	if (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)
> +		die_errno("unable to get credential storage lock");
> +	parse_credential_file(fn, c, NULL, print_line);
> +	if (extra)
> +		print_line(extra);

An entry for a newly updated password comes at the end of the file, instead of replacing an entry already in the file in-place? Given that parse_credential_file() when processing a look-up request (which is the majority of the case) stops upon finding a match, it might make more sense to have the new one (which may be expected to be used often) at the beginning instead, no?

Show 32 quoted lines

> +	if (commit_lock_file(&credential_lock) < 0)
> +		die_errno("unable to commit credential store");
> +}
> +
> +static void store_credential(const char *fn, struct credential *c)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	if (!c->protocol || !(c->host || c->path) ||
> +	    !c->username || !c->password)
> +		return;
> +
> +	strbuf_addf(&buf, "%s://", c->protocol);
> +	strbuf_addstr_urlencode(&buf, c->username, 1);
> +	strbuf_addch(&buf, ':');
> +	strbuf_addstr_urlencode(&buf, c->password, 1);
> +	strbuf_addch(&buf, '@');
> +	if (c->host)
> +		strbuf_addstr_urlencode(&buf, c->host, 1);
> +	if (c->path) {
> +		strbuf_addch(&buf, '/');
> +		strbuf_addstr_urlencode(&buf, c->path, 0);
> +	}
> +
> +	rewrite_credential_file(fn, c, &buf);
> +	strbuf_release(&buf);
> +}
> +
> +static void remove_credential(const char *fn, struct credential *c)
> +{
> +	if (!c->protocol || !(c->host || c->path))
> +		return;

The choice of the fields looks rather arbitrary. I cannot say "remove all the credentials whose username is 'gitster' at 'github.com' no matter what protocol is used", but I can say "remove all credentials under any name for any host as long as the transfer goes over 'https' and accesses a repository at 'if/xyzzy' path", it seems.

This filtering matches what lookup_credential() does but shouldn't it be implemented at a single place in any case?

Show 10 quoted lines

> +	rewrite_credential_file(fn, c, NULL);
> +}
> +
> +static int lookup_credential(const char *fn, struct credential *c)
> +{
> +	if (!c->protocol || !(c->host || c->path))
> +		return 0;
> +	parse_credential_file(fn, c, print_entry, NULL);
> +	return c->username && c->password;
> +}