From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: git@vger•kernel.org
Subject: Re: [PATCH 03/13] introduce credentials API
Date: Tue, 29 Nov 2011 09:34:54 -0800 [thread overview]
Message-ID: <7vmxbeu91d.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <20111129050425.GA32022@sigill.intra.peff.net> (Jeff King's message of "Tue, 29 Nov 2011 00:04:25 -0500")
Jeff King <peff@peff•net> writes:
> On Mon, Nov 28, 2011 at 01:46:35PM -0800, Junio C Hamano wrote:
>
>> > diff --git a/Documentation/technical/api-credentials.txt b/Documentation/technical/api-credentials.txt
>> > new file mode 100644
>> > index 0000000..3061077
>> > --- /dev/null
>> > +++ b/Documentation/technical/api-credentials.txt
>> > @@ -0,0 +1,148 @@
>> > + ...
>> > +`credential_fill`::
>> > +
>> > + Attempt to fill the username and password fields of the passed
>> > + credential struct, first consulting storage helpers, then asking
>> > + the user. Guarantees that the username and password fields will
>> > + be filled afterwards (or die() will be called).
>> > +
>> > +`credential_reject`::
>> > +
>> > + Inform the credential subsystem that the provided credentials
>> > + have been rejected. This will notify any storage helpers of the
>> > + rejection (which allows them to, for example, purge the invalid
>> > + credentials from storage), and then clear the username and
>> > + password fields in `struct credential`. It can then be
>> > + `credential_fill`-ed again.
>> > +
>> > +`credential_approve`::
>> > +
>> > + Inform the credential subsystem that the provided credentials
>> > + were successfully used for authentication. This will notify any
>> > + storage helpers of the approval, so that they can store the
>> > + result to be used again.
>>
>> It's a bit hard to read and understand which part of the system calls
>> these and which other part of the system is responsible for implementing
>> them, and how "helper" fits into the picture (perhaps calling some of
>> these interfaces will result in "helper" getting called?).
>
> How about following it with a rough example of how they would be used,
> like:
>
> /*
> * Create a credential with some context; we don't yet know the
> * username or password.
> */
> struct credential c = CREDENTIAL_INIT;
> c.protocol = xstrdup("https");
> c.host = xstrdup("example.com");
> c.path = xstrdup("foo.git");
>
> /*
> * Fill in the username and password fields by contacting helpers
> * and/or asking the user. The function will die if it fails.
> */
> credential_fill(&c);
>
> /*
> * And then finally make our request, reporting back to the credential
> * system on whether it succeeded or failed.
> */
> if (make_http_request(c.host, c.path, c.username, c.password) < 0)
> credential_reject(&c);
> else
> credential_accept(&c);
>
> Does that make it more clear?
Immensely, at least to me. From the perspective of a user of the API, a
call to credential_fill() is to "fill in the credential" in the sense that
"call the function to fill in the credential" but I find it easier to
understand if it were explained to me as "ask the API to fill in the
credential, which may involve helpers to interact with the user--the point
of the API is that the caller does not care how it is done". Same for the
reject/accept calls---the example makes it clear that they are to tell the
decision to reject/accept made by the application to the credential API,
and it is up to the API layer what it does using that decision (like
removing the cached and now stale password).
The above example is a bit too simplistic and misleading, though. You
would call reject only on authentication failure (do not trash stored and
good password upon network being unreachable temporarily or the server
being overloaded).
> So one possible rule would be:
>
> 1. If it starts with "!", clip off the "!" and hand it to the shell.
>
> 2. Otherwise, if is_absolute_path(), hand it to the shell directly.
>
> 3. Otherwise, prepend "git credential-" and hand it to the shell.
>
> I think that is slightly less confusing than the "first word is alnum"
> thing.
Simpler and easier to explain. Good ;-)
> How do you feel about the "values cannot contain a newline" requirement?
In the context of asking username, password, or passphrase, I think "LF is
the end of the line and you cannot have that byte in your response" is
perfectly reasonable. I've yet to find a way to use LF in a passphrase to
unlock my Gnome keychain ;-).
>> > +int credential_read(struct credential *c, FILE *fp)
>> > +{
>> > ...
>> > + c->host = xstrdup(value);
>> > + }
>> > + else if (!strcmp(key, "path")) {
>> > ...
>> > + /* ignore other lines; we don't know what they mean, but
>> > + * this future-proofs us when later versions of git do
>> > + * learn new lines, and the helpers are updated to match */
>>
>> Two style nits.
>
> I'm supposed to guess? ;P
Sorry, but you guessed right.
next prev parent reply other threads:[~2011-11-29 17:35 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-11-24 10:58 [PATCH 0/13] credential helpers, take two Jeff King
2011-11-24 10:58 ` [PATCH 01/13] test-lib: add test_config_global variant Jeff King
2011-11-24 10:59 ` [PATCH 02/13] t5550: fix typo Jeff King
2011-11-24 11:01 ` [PATCH 03/13] introduce credentials API Jeff King
2011-11-28 21:46 ` Junio C Hamano
2011-11-29 5:04 ` Jeff King
2011-11-29 17:34 ` Junio C Hamano [this message]
2011-11-29 21:14 ` Jeff King
2011-11-24 11:01 ` [PATCH 04/13] credential: add function for parsing url components Jeff King
2011-11-24 11:01 ` [PATCH 05/13] http: use credential API to get passwords Jeff King
2011-11-24 11:02 ` [PATCH 06/13] credential: apply helper config Jeff King
2011-11-24 11:02 ` [PATCH 07/13] credential: add credential.*.username Jeff King
2011-11-24 11:03 ` [PATCH 08/13] credential: make relevance of http path configurable Jeff King
2011-11-24 11:05 ` [PATCH 09/13] docs: end-user documentation for the credential subsystem Jeff King
2011-11-24 11:07 ` [PATCH 10/13] credentials: add "cache" helper Jeff King
2011-11-24 14:36 ` Eric Sunshine
2011-11-29 0:42 ` Junio C Hamano
2011-11-29 5:04 ` Jeff King
2011-11-24 11:07 ` [PATCH 11/13] strbuf: add strbuf_add*_urlencode Jeff King
2011-11-29 18:19 ` Junio C Hamano
2011-11-29 21:19 ` Jeff King
2011-11-29 23:26 ` René Scharfe
2011-11-30 3:20 ` Jeff King
2011-11-30 5:40 ` Junio C Hamano
2011-11-30 5:41 ` René Scharfe
2011-11-24 11:07 ` [PATCH 12/13] credentials: add "store" helper Jeff King
2011-11-24 14:29 ` Eric Sunshine
2011-11-24 20:09 ` Jeff King
2011-11-29 18:19 ` Junio C Hamano
2011-11-29 21:38 ` Jeff King
2011-11-24 11:09 ` [PATCH 13/13] t: add test harness for external credential helpers Jeff King
2011-11-24 11:45 ` [PATCH 0/13] credential helpers, take two Erik Faye-Lund
2011-11-24 11:53 ` Jeff King
2011-11-24 12:08 ` Erik Faye-Lund
2011-11-27 8:27 ` [PATCH 0/6] echo usernames as they are typed Jeff King
2011-11-27 8:28 ` [PATCH 1/6] move git_getpass to its own source file Jeff King
2011-11-27 8:29 ` [PATCH 2/6] refactor git_getpass into generic prompt function Jeff King
2011-11-27 8:30 ` [PATCH 3/6] stub out getpass_echo function Jeff King
2011-11-27 8:30 ` [PATCH 4/6] prompt: add PROMPT_ECHO flag Jeff King
2011-11-27 8:31 ` [PATCH 5/6] credential: use git_prompt instead of git_getpass Jeff King
2011-11-27 8:31 ` [PATCH 6/6] compat/getpass: add a /dev/tty implementation Jeff King
2011-11-27 8:56 ` [PATCH 0/6] echo usernames as they are typed Junio C Hamano
2011-11-27 9:17 ` Erik Faye-Lund
2011-11-28 3:53 ` Jeff King
2011-11-28 9:36 ` Erik Faye-Lund
2011-11-28 11:31 ` Jeff King
2011-11-28 11:49 ` Frans Klaver
2011-11-28 12:59 ` Erik Faye-Lund
2011-11-28 18:59 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7vmxbeu91d.fsf@alter.siamese.dyndns.org \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox