Re: [PATCH 06/14] introduce credentials API
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 20, 2011, 22:17 UTC
- Message-ID
- <7vy5zs4mxt.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20110718075034.GF12341@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> +`struct credential`:: > + > + This struct represents a single username/password combination. > + The `username` and `password` fields should be heap-allocated > + strings (or NULL if they are not yet known). The `unique` field, > + if non-NULL, should be a heap-allocated string indicating a > + unique context for this credential (e.g., a protocol and server > + name for a remote credential). The `description` field, if > + non-NULL, should point to a string containing a human-readable > + description of this credential.
Am I confused to say "unique" is for code/machine and "description" is for human to identify what remote resource is being accessed using the credential? Can two credentials have the same description but different unique, and if so what happens? A plausible answer may be "the user cannot tell what username/password pair to give, but it is expected that such a nondescriptive context is only for certificate and it also is expected that only one certificate is used at a time", perhaps?
I think "unique" vs "description" are secondary aspects of these things, and the primary aspect that they share is that they are about "context", as [11/14] describes. Perhaps "context-id" vs "context-description" may be better names to reduce confusion? I dunno.
Show 12 quoted lines
> +`credential_fill`:: > + > + Like `credential_fill_gently`, but `die()` if credentials cannot > + be gathered. > + > +`credential_reject`:: > + > + Inform the credential subsystem that the provided credentials > + have been rejected. This will clear the username and password > + fields in `struct credential`, as well as notify any helpers of > + the rejection (which may, for example, purge the invalid > + credentials from storage).
What hints do helpers get when this is called? Do they get username, unique and description to allow them to selectively purge the bad ones while keeping good ones, or the username is already cleared by the time the helpers are notified and they have no clue?
Show 6 quoted lines
> +`credential_getpass`:: > + > + Fetch credentials from the user either using an "askpass" helper > + (see the discussion of core.askpass and GIT_ASKPASS in > + linkgit:git-config[1] and linkgit:git[1], respectively) or by > + prompting the user via the terminal.
It sounds like that users of the API should call fill_credential() and let the machinery fall back to this function as needed. Does this need to be part of the public API functions?
Show 18 quoted lines
> +static int read_credential_response(struct credential *c, FILE *fp)
> +{
> + struct strbuf response = STRBUF_INIT;
> +
> + while (strbuf_getline(&response, fp, '\n') != EOF) {
> + char *key = response.buf;
> + char *value = strchr(key, '=');
> +
> + if (!value) {
> + warning("bad output from credential helper: %s", key);
> + strbuf_release(&response);
> + return -1;
> + }
> + *value++ = '\0';
> +
> + if (!strcmp(key, "username")) {
> + free(c->username);
> + c->username = xstrdup(value);The document did not say anything about escaping/quoting of values, but it may not be a bad idea to make it more explicit over there.