Re: [PATCH 12/13] credentials: add "store" helper
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:52:32
Jeff King [off-list ref] writes:
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ if (match_cb) { + match_cb(&entry); + break;
Multiple matches not allowed, which sounds fine.
quoted hunk ↗ jump to hunk
+ } + } + 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.
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
+ 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; +}