threads / patch / 31049

patch, 4 partsmediawiki/credential cleanups

Subject: [PATCH 0/4] mediawiki/credential cleanups

## tl;dr

14 messages between Jul 18, 2012 and Jul 18, 2012. Diffs are folded; open one to read it.

replies: 13people: 2as markdown or json

Jeff King· Jul 18, 2012, 12:03 UTC · lore

Hi Matthieu,

Your initial credential plumbing series went by while I was on vacation, and I finally got a chance to look at it in more detail. Overall it looks very good; thank you (and your students) for working on it. It was especially nice to have a test harness with complete instructions. :)

I came up with a few commits on top. Patches 1 and 2 are cleanups that should hopefully be non-controversial. Patch 3 is the "url=" attribute we discussed earlier, and then patch 4 makes use of it in mw-to-git.

  [1/4]: docs/credential: minor clarity fixups
  [2/4]: mw-to-git: check blank credential attributes via length
  [3/4]: credential: convert "url" attribute into its parsed subparts
  [4/4]: mw-to-git: use git-credential's URL parser

 Documentation/git-credential.txt       | 22 ++++++++++++++++------
 contrib/mw-to-git/git-remote-mediawiki | 34 +++++-----------------------------
 credential.c                           |  2 ++
 3 files changed, 23 insertions(+), 35 deletions(-)

-Peff

Jeff King· Jul 18, 2012, 12:04 UTC · re: Jeff King · lore

[PATCH 1/4] docs/credential: minor clarity fixups

The text in git-credential(1) was copied from technical/api-credentials, so it still talks about the input/output format as coming from git to the helper. Since the surrounding text already indicates that this format is used for reading and writing with git credential, we can just remove the extraneous confusing bits.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-credential.txt | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Show changes to Documentation/git-credential.txt +4 −6

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index a81684e..afd5365 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -102,22 +102,20 @@ INPUT/OUTPUT FORMAT
 -------------------
 
 `git credential` reads and/or writes (depending on the action used)
-credential information in its standard input/output. These information
+credential information in its standard input/output. This information
 can correspond either to keys for which `git credential` will obtain
 the login/password information (e.g. host, protocol, path), or to the
 actual credential data to be obtained (login/password).
 
-The credential is split into a set of named attributes.
-Attributes are provided to the helper, one per line. Each attribute is
+The credential is split into a set of named attributes, with one
+attribute per line. Each attribute is
 specified by a key-value pair, separated by an `=` (equals) sign,
 followed by a newline. The key may contain any bytes except `=`,
 newline, or NUL. The value may contain any bytes except newline or NUL.
 In both cases, all bytes are treated as-is (i.e., there is no quoting,
 and one cannot transmit a value with newline or NUL in it). The list of
 attributes is terminated by a blank line or end-of-file.
-Git will send the following attributes (but may not send all of
-them for a given credential; for example, a `host` attribute makes no
-sense when dealing with a non-network protocol):
+Git understands the following attributes:
 
 `protocol`::

-- 
1.7.10.5.40.g059818d

Jeff King· Jul 18, 2012, 12:04 UTC · re: Jeff King · lore

[PATCH 2/4] mw-to-git: check blank credential attributes via length

When writing a credential to git-credential, we omit fields that do not have a true value. This will skip empty or undefined fields (which we want), but will also accidentally skip usernames or passwords which happen to have a non-true value (e.g., "0"). Be more careful by checking for non-zero length.

Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/mw-to-git/git-remote-mediawiki | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to contrib/mw-to-git/git-remote-mediawiki +1 −1

diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki
index accd70a..b06f27b 100755
--- a/contrib/mw-to-git/git-remote-mediawiki
+++ b/contrib/mw-to-git/git-remote-mediawiki
@@ -207,7 +207,7 @@ sub credential_write {
 	my $credential = shift;
 	my $writer = shift;
 	while (my ($key, $value) = each(%$credential) ) {
-		if ($value) {
+		if (length $value) {
 			print $writer "$key=$value\n";
 		}
 	}

-- 
1.7.10.5.40.g059818d

Jeff King· Jul 18, 2012, 12:06 UTC · re: Jeff King · lore

[PATCH 3/4] credential: convert "url" attribute into its parsed subparts

The git-credential command requires that you feed it a broken-down credential, which means that the client needs to parse a URL itself. Since we have our own URL-parsing routines, we can easily allow the caller to just give us the URL as-is, saving them some code.

Signed-off-by: Jeff King <peff@peff.net>
---
The implementation turned out to be delightfully simple. I stopped short
of adding an "ident" command to git-credential where you could do
something like:

  $ echo https://user@example.com | git credential ident
  protocol=https
  host=example.com
  username=user

since I had no use for it, but it would obviously be an easy one-liner to write (it's just "fill" without the actual fill call).

 Documentation/git-credential.txt | 12 ++++++++++++
 credential.c                     |  2 ++
 2 files changed, 14 insertions(+)

Show changes to 2 files +14 −0

Documentation/git-credential.txt, credential.c

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index afd5365..53adee3 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -140,3 +140,15 @@ Git understands the following attributes:
 `password`::
 
 	The credential's password, if we are asking it to be stored.
+
+`url`::
+
+	When this special attribute is read by `git credential`, the
+	value is parsed as a URL and treated as if its constituent parts
+	were read (e.g., `url=https://example.com` would behave as if
+	`protocol=https` and `host=example.com` had been provided). This
+	can help callers avoid parsing URLs themselves.  Note that any
+	components which are missing from the URL (e.g., there is no
+	username in the example above) will be set to empty; if you want
+	to provide a URL and override some attributes, provide the URL
+	attribute first, followed by any overrides.
diff --git a/credential.c b/credential.c
index 2c40007..e54753c 100644
--- a/credential.c
+++ b/credential.c
@@ -172,6 +172,8 @@ int credential_read(struct credential *c, FILE *fp)
 		} else if (!strcmp(key, "path")) {
 			free(c->path);
 			c->path = xstrdup(value);
+		} else if (!strcmp(key, "url")) {
+			credential_from_url(c, value);
 		}
 		/*
 		 * Ignore other lines; we don't know what they mean, but

-- 
1.7.10.5.40.g059818d

Matthieu Moy· Jul 18, 2012, 12:24 UTC · re: Jeff King · lore

Re: [PATCH 3/4] credential: convert "url" attribute into its parsed subparts

Jeff King <peff@peff.net> writes:

Show 7 quoted lines

>   $ echo https://user@example.com | git credential ident
>   protocol=https
>   host=example.com
>   username=user
>
> since I had no use for it, but it would obviously be an easy one-liner
> to write (it's just "fill" without the actual fill call).

I was thinking the same, except I would have spelled it "git credential parse" (but ident is fine too). On the perl side, that would allow getting a credential hash very simply (but it was already simple in perl, and made useless by your code).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Jeff King· Jul 18, 2012, 12:25 UTC · re: Matthieu Moy · lore

Re: [PATCH 3/4] credential: convert "url" attribute into its parsed subparts

On Wed, Jul 18, 2012 at 02:24:01PM +0200, Matthieu Moy wrote:

Show 14 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> >   $ echo https://user@example.com | git credential ident
> >   protocol=https
> >   host=example.com
> >   username=user
> >
> > since I had no use for it, but it would obviously be an easy one-liner
> > to write (it's just "fill" without the actual fill call).
> 
> I was thinking the same, except I would have spelled it "git credential
> parse" (but ident is fine too). On the perl side, that would allow
> getting a credential hash very simply (but it was already simple in
> perl, and made useless by your code).

I wanted to give it some name that meant "pass-through" rather than just parse, in case we add more magic attributes later. I meant "ident" to be like "mathematical identity", but in the context of a credential tool, it is probably somewhat ambiguous. :)

Anyway, the fact that we can just do the parsing as part of the "fill" means we don't need it for now, so I'll leave it until somebody really cares.

-Peff

Jeff King· Jul 18, 2012, 12:06 UTC · re: Jeff King · lore

[PATCH 4/4] mw-to-git: use git-credential's URL parser

We can just feed our URL straight to git-credential and it will parse it for us, saving us some code.

Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/mw-to-git/git-remote-mediawiki | 32 ++++----------------------------
 1 file changed, 4 insertions(+), 28 deletions(-)

Show changes to contrib/mw-to-git/git-remote-mediawiki +4 −28

diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki
index b06f27b..c9ac416 100755
--- a/contrib/mw-to-git/git-remote-mediawiki
+++ b/contrib/mw-to-git/git-remote-mediawiki
@@ -163,32 +163,6 @@ while (<STDIN>) {
 
 ## credential API management (generic functions)
 
-sub credential_from_url {
-	my $url = shift;
-	my $parsed = URI->new($url);
-	my %credential;
-
-	if ($parsed->scheme) {
-		$credential{protocol} = $parsed->scheme;
-	}
-	if ($parsed->host) {
-		$credential{host} = $parsed->host;
-	}
-	if ($parsed->path) {
-		$credential{path} = $parsed->path;
-	}
-	if ($parsed->userinfo) {
-		if ($parsed->userinfo =~ /([^:]*):(.*)/) {
-			$credential{username} = $1;
-			$credential{password} = $2;
-		} else {
-			$credential{username} = $parsed->userinfo;
-		}
-	}
-
-	return %credential;
-}
-
 sub credential_read {
 	my %credential;
 	my $reader = shift;
@@ -216,7 +190,9 @@ sub credential_write {
 sub credential_run {
 	my $op = shift;
 	my $credential = shift;
+	my $url = shift;
 	my $pid = open2(my $reader, my $writer, "git credential $op");
+	print $writer "url=$url\n" if defined $url;
 	credential_write($credential, $writer);
 	print $writer "\n";
 	close($writer);
@@ -246,10 +222,10 @@ sub mw_connect_maybe {
 	$mediawiki = MediaWiki::API->new;
 	$mediawiki->{config}->{api_url} = "$url/api.php";
 	if ($wiki_login) {
-		my %credential = credential_from_url($url);
+		my %credential;
 		$credential{username} = $wiki_login;
 		$credential{password} = $wiki_passwd;
-		credential_run("fill", \%credential);
+		credential_run("fill", \%credential, $url);
 		my $request = {lgname => $credential{username},
 			       lgpassword => $credential{password},
 			       lgdomain => $wiki_domain};

-- 
1.7.10.5.40.g059818d

Matthieu Moy· Jul 18, 2012, 12:24 UTC · re: Jeff King · lore

Re: [PATCH 4/4] mw-to-git: use git-credential's URL parser

Jeff King <peff@peff.net> writes:

Show 23 quoted lines

> @@ -216,7 +190,9 @@ sub credential_write {
>  sub credential_run {
>  	my $op = shift;
>  	my $credential = shift;
> +	my $url = shift;
>  	my $pid = open2(my $reader, my $writer, "git credential $op");
> +	print $writer "url=$url\n" if defined $url;
>  	credential_write($credential, $writer);
>  	print $writer "\n";
>  	close($writer);
> @@ -246,10 +222,10 @@ sub mw_connect_maybe {
>  	$mediawiki = MediaWiki::API->new;
>  	$mediawiki->{config}->{api_url} = "$url/api.php";
>  	if ($wiki_login) {
> -		my %credential = credential_from_url($url);
> +		my %credential;
>  		$credential{username} = $wiki_login;
>  		$credential{password} = $wiki_passwd;
> -		credential_run("fill", \%credential);
> +		credential_run("fill", \%credential, $url);
>  		my $request = {lgname => $credential{username},
>  			       lgpassword => $credential{password},
>  			       lgdomain => $wiki_domain};

I would find it more elegant not to special-case the URL field, and just

  my %credential = ('url' => $url);

but I'm fine with your version too.

Other than that, and for the 4 patches of the serie:

Reviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Jeff King· Jul 18, 2012, 12:28 UTC · re: Matthieu Moy · lore

Re: [PATCH 4/4] mw-to-git: use git-credential's URL parser

On Wed, Jul 18, 2012 at 02:24:13PM +0200, Matthieu Moy wrote:

Show 31 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > @@ -216,7 +190,9 @@ sub credential_write {
> >  sub credential_run {
> >  	my $op = shift;
> >  	my $credential = shift;
> > +	my $url = shift;
> >  	my $pid = open2(my $reader, my $writer, "git credential $op");
> > +	print $writer "url=$url\n" if defined $url;
> >  	credential_write($credential, $writer);
> >  	print $writer "\n";
> >  	close($writer);
> > @@ -246,10 +222,10 @@ sub mw_connect_maybe {
> >  	$mediawiki = MediaWiki::API->new;
> >  	$mediawiki->{config}->{api_url} = "$url/api.php";
> >  	if ($wiki_login) {
> > -		my %credential = credential_from_url($url);
> > +		my %credential;
> >  		$credential{username} = $wiki_login;
> >  		$credential{password} = $wiki_passwd;
> > -		credential_run("fill", \%credential);
> > +		credential_run("fill", \%credential, $url);
> >  		my $request = {lgname => $credential{username},
> >  			       lgpassword => $credential{password},
> >  			       lgdomain => $wiki_domain};
> 
> I would find it more elegant not to special-case the URL field, and just
> 
>   my %credential = ('url' => $url);
> 
> but I'm fine with your version too.

I started with a version that did that, but there are two complications:

  1. credential_write needs to know that the 'url' field must come
     first, as it overwrites the other fields. So we end up
     special-casing it either way.

  2. Git hands us back the broken-down version, which we add to the
     credential. So it is superfluous to keep sending 'url' for the
     other, non-fill interactions. I don't think it's technically wrong,
     but it also seemed inelegant. Having "credential_write" delete the
     'url' field seemed even more inelegant.

> Other than that, and for the 4 patches of the serie:
> 
> Reviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>

Thanks.

-Peff

Matthieu Moy· Jul 18, 2012, 12:37 UTC · re: Jeff King · lore

Re: [PATCH 4/4] mw-to-git: use git-credential's URL parser

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

> I started with a version that did that, but there are two complications:
>
>   1. credential_write needs to know that the 'url' field must come
>      first, as it overwrites the other fields. So we end up
>      special-casing it either way.

Right, I didn't think of that. But you'd have to special-case it only within credential_run, and not for the caller.

>   2. Git hands us back the broken-down version, which we add to the
>      credential.

We don't add it, but already replace the whole structure. This is somehow needed because "git credential fill" can remove fields from the structure (the path attribute is removed with credential.useHttpPath=false). So, this point doesn't seem problematic.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Jeff King· Jul 18, 2012, 12:57 UTC · re: Matthieu Moy · lore

Re: [PATCH 4/4] mw-to-git: use git-credential's URL parser

On Wed, Jul 18, 2012 at 02:37:27PM +0200, Matthieu Moy wrote:

Show 10 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > I started with a version that did that, but there are two complications:
> >
> >   1. credential_write needs to know that the 'url' field must come
> >      first, as it overwrites the other fields. So we end up
> >      special-casing it either way.
> 
> Right, I didn't think of that. But you'd have to special-case it only
> within credential_run, and not for the caller.

Yeah. It would look like this:

Show changes to contrib/mw-to-git/git-remote-mediawiki +6 −6

diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki
index c9ac416..e1392b0 100755
--- a/contrib/mw-to-git/git-remote-mediawiki
+++ b/contrib/mw-to-git/git-remote-mediawiki
@@ -190,9 +190,11 @@ sub credential_write {
 sub credential_run {
 	my $op = shift;
 	my $credential = shift;
-	my $url = shift;
 	my $pid = open2(my $reader, my $writer, "git credential $op");
-	print $writer "url=$url\n" if defined $url;
+	if (exists $credential->{url}) {
+		print $writer "url=$credential->{url}\n";
+		delete $credential->{url};
+	}
 	credential_write($credential, $writer);
 	print $writer "\n";
 	close($writer);

which is still kind of ugly. We could also push it down into
credential_write, like:

diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki
index c9ac416..0a821fd 100755
--- a/contrib/mw-to-git/git-remote-mediawiki
+++ b/contrib/mw-to-git/git-remote-mediawiki
@@ -180,8 +180,9 @@ sub credential_read {
 sub credential_write {
 	my $credential = shift;
 	my $writer = shift;
+	print $writer "url=$credential->{url}\n" if exists $credential->{url};
 	while (my ($key, $value) = each(%$credential) ) {
-		if (length $value) {
+		if (length $value && $key ne 'url') {
 			print $writer "$key=$value\n";
 		}
 	}
@@ -190,9 +191,7 @@ sub credential_write {
 sub credential_run {
 	my $op = shift;
 	my $credential = shift;
-	my $url = shift;
 	my $pid = open2(my $reader, my $writer, "git credential $op");
-	print $writer "url=$url\n" if defined $url;
 	credential_write($credential, $writer);
 	print $writer "\n";
 	close($writer);

which is probably the least gross. I was originally hesitant because the
issue (2) I brought up, but...

> >   2. Git hands us back the broken-down version, which we add to the
> >      credential.
> 
> We don't add it, but already replace the whole structure. This is
> somehow needed because "git credential fill" can remove fields from the
> structure (the path attribute is removed with
> credential.useHttpPath=false). So, this point doesn't seem problematic.

Hmph. I considered that we might do it and even checked, but I somehow
read the code wrong (I think I was thrown off by the pass-by-reference
to credential_run, but of course it overwrites it inside that function).

So since that is a non-issue, I think the second diff I provided above
is a bit nicer.

-Peff

Jeff King· Jul 18, 2012, 13:03 UTC · re: Jeff King · lore

[PATCHv2 4/4] mw-to-git: use git-credential's URL parser

On Wed, Jul 18, 2012 at 08:57:41AM -0400, Jeff King wrote:

> So since that is a non-issue, I think the second diff I provided above
> is a bit nicer.

And here is a replacement patch 4/4.

-- >8 --
Subject: mw-to-git: use git-credential's URL parser

We can just feed our URL straight to git-credential and it will parse it for us, saving us some code.

Signed-off-by: Jeff King <peff@peff.net>
---
 contrib/mw-to-git/git-remote-mediawiki | 32 ++++----------------------------
 1 file changed, 4 insertions(+), 28 deletions(-)

Show changes to contrib/mw-to-git/git-remote-mediawiki +4 −28

diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki
index b06f27b..38afa76 100755
--- a/contrib/mw-to-git/git-remote-mediawiki
+++ b/contrib/mw-to-git/git-remote-mediawiki
@@ -163,32 +163,6 @@ while (<STDIN>) {
 
 ## credential API management (generic functions)
 
-sub credential_from_url {
-	my $url = shift;
-	my $parsed = URI->new($url);
-	my %credential;
-
-	if ($parsed->scheme) {
-		$credential{protocol} = $parsed->scheme;
-	}
-	if ($parsed->host) {
-		$credential{host} = $parsed->host;
-	}
-	if ($parsed->path) {
-		$credential{path} = $parsed->path;
-	}
-	if ($parsed->userinfo) {
-		if ($parsed->userinfo =~ /([^:]*):(.*)/) {
-			$credential{username} = $1;
-			$credential{password} = $2;
-		} else {
-			$credential{username} = $parsed->userinfo;
-		}
-	}
-
-	return %credential;
-}
-
 sub credential_read {
 	my %credential;
 	my $reader = shift;
@@ -206,8 +180,10 @@ sub credential_read {
 sub credential_write {
 	my $credential = shift;
 	my $writer = shift;
+	# url overwrites other fields, so it must come first
+	print $writer "url=$credential->{url}\n" if exists $credential->{url};
 	while (my ($key, $value) = each(%$credential) ) {
-		if (length $value) {
+		if (length $value && $key ne 'url') {
 			print $writer "$key=$value\n";
 		}
 	}
@@ -246,7 +222,7 @@ sub mw_connect_maybe {
 	$mediawiki = MediaWiki::API->new;
 	$mediawiki->{config}->{api_url} = "$url/api.php";
 	if ($wiki_login) {
-		my %credential = credential_from_url($url);
+		my %credential = (url => $url);
 		$credential{username} = $wiki_login;
 		$credential{password} = $wiki_passwd;
 		credential_run("fill", \%credential);

-- 
1.7.10.5.40.g059818d

Matthieu Moy· Jul 18, 2012, 13:06 UTC · re: Jeff King · lore

Re: [PATCHv2 4/4] mw-to-git: use git-credential's URL parser

Jeff King <peff@peff.net> writes:

Show 9 quoted lines

> On Wed, Jul 18, 2012 at 08:57:41AM -0400, Jeff King wrote:
>
>> So since that is a non-issue, I think the second diff I provided above
>> is a bit nicer.
>
> And here is a replacement patch 4/4.
>
> -- >8 --
> Subject: mw-to-git: use git-credential's URL parser

Perfect, thanks.

(and you've been faster than me about the comment ;-) )

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Matthieu Moy· Jul 18, 2012, 13:05 UTC · re: Jeff King · lore

Re: [PATCH 4/4] mw-to-git: use git-credential's URL parser

Jeff King <peff@peff.net> writes:

> So since that is a non-issue, I think the second diff I provided above
> is a bit nicer.

I like this one best too. It just lacks a comment saying why it should be displayed first ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/