* [PATCH_v1] add git credential login to remote mediawiki
@ 2012-06-09 18:53 Javier.Roucher-Iglesias
2012-06-10 6:54 ` Junio C Hamano
2012-06-10 12:18 ` Jeff King
0 siblings, 2 replies; 6+ messages in thread
From: Javier.Roucher-Iglesias @ 2012-06-09 18:53 UTC (permalink / raw)
To: git
Cc: Javier Roucher, Pavel Volek, NGUYEN Kim Thuat,
ROUCHER IGLESIAS Javier, Matthieu Moy
From: Javier Roucher <jroucher@gmail.com>
This path uses git credential to store the login/password of the mediawiki.
Signed-off-by: Pavel Volek <Pavel.Volek@ensimag.imag.fr>
Signed-off-by: NGUYEN Kim Thuat <Kim-Thuat.Nguyen@ensimag.imag.fr>
Signed-off-by: ROUCHER IGLESIAS Javier <roucherj@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
contrib/mw-to-git/git-remote-mediawiki | 107 +++++++++++++++++++++++++++++----
1 file changed, 95 insertions(+), 12 deletions(-)
diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki
index c18bfa1..4b14d78 100755
--- a/contrib/mw-to-git/git-remote-mediawiki
+++ b/contrib/mw-to-git/git-remote-mediawiki
@@ -152,28 +152,111 @@ while (<STDIN>) {
########################## Functions ##############################
# MediaWiki API instance, created lazily.
+sub run_credential {
+ my $cre_protocol = "";
+ my $cre_host = "";
+ my $cre_path = "";
+ my $msg = "";
+ my $result = "";
+ my $op=$_[0];
+ if (scalar(@_) == 2) {
+ if ($_[1] eq ("store" || "cache")) {
+ run_git("config credential.helper \'$_[1]\'");
+ } else {
+ print STDERR "ERROR: run_credential (fill|approve|reject) [store|cache]\n";
+ exit 1;
+ }
+ }
+ my $parsed = URI->new($url);
+ $cre_protocol = $parsed->scheme;
+ $cre_host = $parsed->host;
+ $cre_path = $parsed->path;
+
+ if ($wiki_login ne "") {
+ $msg .= "username=$wiki_login\n";
+ }
+ if ($wiki_passwd ne "") {
+ $msg .= "password=$wiki_passwd\n";
+ }
+ if ($cre_protocol ne "") {
+ $msg .= "protocol=$cre_protocol\n";
+ }
+ if ($cre_host ne "") {
+ $msg .= "host=$cre_host\n";
+ }
+ if ($cre_path ne "") {
+ $msg .= "path=$cre_path\n";
+ }
+
+ $msg .= "\n";
+
+ my $key;
+ my $value;
+ my $Prog = "git credential $op";
+ open2(*Reader, *Writer, $Prog);
+ print Writer $msg;
+ close (Writer);
+
+ if ($op eq "fill") {
+ while (<Reader>) {
+ my ($key, $value) = /([^=]*)=(.*)/;
+ # error if key undef
+ if (not defined $key) {
+ print STDERR "ERROR reciving reponse git credential fill\n";
+ exit 1;
+ }
+ if ($key eq "username") {
+ $wiki_login = $value;
+ }
+ if ($key eq "password") {
+ $wiki_passwd = $value;
+ }
+ }
+ } else {
+ while (<Reader>) {
+ print STDERR "\nERROR while running git credential $op:\n$_";
+ }
+ }
+}
+
my $mediawiki;
+sub ask_login {
+ run_credential("fill","store");
+
+ if (!$mediawiki->login( {
+ lgname => $wiki_login,
+ lgpassword => $wiki_passwd,
+ lgdomain => $wiki_domain,
+ } )) {
+ print STDERR "Failed to log in mediawiki user \"$wiki_login\" on $url\n";
+ print STDERR "URL:$wiki_domain $url\n";
+ print STDERR "(error " .
+ $mediawiki->{error}->{code} . ': ' .
+ $mediawiki->{error}->{details} . ")\n";
+ run_credential("reject");
+ # exit 1;
+ } else {
+ print STDERR "Logged in with user \"$wiki_login\".\n";
+ run_credential("approve");
+ }
+}
+
sub mw_connect_maybe {
+
if ($mediawiki) {
return;
}
$mediawiki = MediaWiki::API->new;
$mediawiki->{config}->{api_url} = "$url/api.php";
if ($wiki_login) {
- if (!$mediawiki->login({
- lgname => $wiki_login,
- lgpassword => $wiki_passwd,
- lgdomain => $wiki_domain,
- })) {
- print STDERR "Failed to log in mediawiki user \"$wiki_login\" on $url\n";
- print STDERR "(error " .
- $mediawiki->{error}->{code} . ': ' .
- $mediawiki->{error}->{details} . ")\n";
- exit 1;
- } else {
- print STDERR "Logged in with user \"$wiki_login\".\n";
+ if (!$wiki_passwd) {
+ #user knows, password not.
+ ask_login();
}
+ } else {
+ #user or password not knows
+ ask_login();
}
}
--
1.7.11.rc2.9.ge2c5c96.dirty
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH_v1] add git credential login to remote mediawiki
2012-06-09 18:53 [PATCH_v1] add git credential login to remote mediawiki Javier.Roucher-Iglesias
@ 2012-06-10 6:54 ` Junio C Hamano
2012-06-10 12:18 ` Jeff King
1 sibling, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2012-06-10 6:54 UTC (permalink / raw)
To: Javier.Roucher-Iglesias
Cc: git, Javier Roucher, Pavel Volek, NGUYEN Kim Thuat,
ROUCHER IGLESIAS Javier, Matthieu Moy
Javier.Roucher-Iglesias@ensimag.imag.fr writes:
> Subject: [PATCH_v1] add git credential login to remote mediawiki
Please remove "_" between "PATCH" and "v1".
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH_v1] add git credential login to remote mediawiki
2012-06-09 18:53 [PATCH_v1] add git credential login to remote mediawiki Javier.Roucher-Iglesias
2012-06-10 6:54 ` Junio C Hamano
@ 2012-06-10 12:18 ` Jeff King
2012-06-10 13:37 ` Matthieu Moy
1 sibling, 1 reply; 6+ messages in thread
From: Jeff King @ 2012-06-10 12:18 UTC (permalink / raw)
To: Javier.Roucher-Iglesias
Cc: git, Javier Roucher, Pavel Volek, NGUYEN Kim Thuat,
ROUCHER IGLESIAS Javier, Matthieu Moy
On Sat, Jun 09, 2012 at 08:53:48PM +0200, Javier.Roucher-Iglesias@ensimag.imag.fr wrote:
> diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki
> index c18bfa1..4b14d78 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki
> +++ b/contrib/mw-to-git/git-remote-mediawiki
> @@ -152,28 +152,111 @@ while (<STDIN>) {
> ########################## Functions ##############################
>
> # MediaWiki API instance, created lazily.
> +sub run_credential {
Is there any reason not to add this to perl/Git.pm? I suspect that other
scripts will want to use it, too (for example, send-email could probably
use it for SMTP credentials).
> + if (scalar(@_) == 2) {
> + if ($_[1] eq ("store" || "cache")) {
> + run_git("config credential.helper \'$_[1]\'");
> + } else {
> + print STDERR "ERROR: run_credential (fill|approve|reject) [store|cache]\n";
> + exit 1;
> + }
> + }
This hunk looks wrong. You should never be setting the credential.helper
config; that is the responsibility of the user to set, as they want to
select whatever helper is appropriate. Nor do you need to care about
which helpers are in use; the point of git-credential is that it will do
that for you.
> + my $parsed = URI->new($url);
> + $cre_protocol = $parsed->scheme;
> + $cre_host = $parsed->host;
> + $cre_path = $parsed->path;
> +
> + if ($wiki_login ne "") {
> + $msg .= "username=$wiki_login\n";
> + }
> + if ($wiki_passwd ne "") {
> + $msg .= "password=$wiki_passwd\n";
> + }
> + if ($cre_protocol ne "") {
> + $msg .= "protocol=$cre_protocol\n";
> + }
> + if ($cre_host ne "") {
> + $msg .= "host=$cre_host\n";
> + }
> + if ($cre_path ne "") {
> + $msg .= "path=$cre_path\n";
> + }
> +
> + $msg .= "\n";
All of this could just go away for the "fill" case if we allow URLs on
the command line (see my previous email if you haven't already). And for
the "approve" and "reject" cases, we could just save the result from
"fill" and feed it back verbatim, as I described in the earlier email.
Then it would be as simple as:
sub fill_credential {
my $quoted_url = quotemeta(shift);
my $verbatim = `git credential fill $quoted_url`;
$? and die "git-credential failed";
$verbatim =~ /^username=(.*)$/m
or die "git-credential did not give us a username";
my $username = $1;
$verbatim =~ /^password=(.*)$/m
or die "git-credential did not give us a password";
return ($username, $password, $verbatim);
}
sub report_credential {
my ($type, $verbatim) = @_;
open(my $fh, '|-', "git credential $type");
print $fh $verbatim;
}
> + my $key;
> + my $value;
> + my $Prog = "git credential $op";
> + open2(*Reader, *Writer, $Prog);
> + print Writer $msg;
> + close (Writer);
> +
> + if ($op eq "fill") {
> + while (<Reader>) {
> + my ($key, $value) = /([^=]*)=(.*)/;
> + # error if key undef
> + if (not defined $key) {
> + print STDERR "ERROR reciving reponse git credential fill\n";
> + exit 1;
> + }
> + if ($key eq "username") {
> + $wiki_login = $value;
> + }
> + if ($key eq "password") {
> + $wiki_passwd = $value;
> + }
> + }
> + } else {
> + while (<Reader>) {
> + print STDERR "\nERROR while running git credential $op:\n$_";
> + }
> + }
> +}
This isn't a good way to check for errors. The non-fill actions will
never produce output on stdout, and you are not intercepting their
stderr. Besides which, checking for errors by reading stderr is not a
good practice; you should check the return value of the command in $?
after it finishes.
-Peff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH_v1] add git credential login to remote mediawiki
2012-06-10 12:18 ` Jeff King
@ 2012-06-10 13:37 ` Matthieu Moy
2012-06-10 17:36 ` roucherj
2012-06-10 18:39 ` Jeff King
0 siblings, 2 replies; 6+ messages in thread
From: Matthieu Moy @ 2012-06-10 13:37 UTC (permalink / raw)
To: Jeff King
Cc: Javier.Roucher-Iglesias, git, Javier Roucher, Pavel Volek,
NGUYEN Kim Thuat, ROUCHER IGLESIAS Javier
Jeff King <peff@peff.net> writes:
> On Sat, Jun 09, 2012 at 08:53:48PM +0200, Javier.Roucher-Iglesias@ensimag.imag.fr wrote:
>
>> diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki
>> index c18bfa1..4b14d78 100755
>> --- a/contrib/mw-to-git/git-remote-mediawiki
>> +++ b/contrib/mw-to-git/git-remote-mediawiki
>> @@ -152,28 +152,111 @@ while (<STDIN>) {
>> ########################## Functions ##############################
>>
>> # MediaWiki API instance, created lazily.
>> +sub run_credential {
>
> Is there any reason not to add this to perl/Git.pm? I suspect that other
> scripts will want to use it, too (for example, send-email could probably
> use it for SMTP credentials).
Currently, git-remote-mediawiki is a standalone script (doesn't use
Git.pm). This is good because it makes it trivial to install, but bad in
the sense that it may force us (or others) to reinvent the wheel.
Until now, the wheels we reinvented were very simple (run_git
essentially), but we may be reaching the point where it makes sense to
use and contribute to Git.pm.
Unfortunately, from a non-technical point of view, Javier is
contributing this as part of a student project, which ends this week,
and it's probably not reasonable to introduce such change so late. So,
I'd keep it here at least for now, and a move to Git.pm could be a
separate future topic.
>> + if (scalar(@_) == 2) {
>> + if ($_[1] eq ("store" || "cache")) {
>> + run_git("config credential.helper \'$_[1]\'");
>> + } else {
>> + print STDERR "ERROR: run_credential (fill|approve|reject) [store|cache]\n";
>> + exit 1;
>> + }
>> + }
>
> This hunk looks wrong. You should never be setting the credential.helper
> config; that is the responsibility of the user to set, as they want to
> select whatever helper is appropriate. Nor do you need to care about
> which helpers are in use; the point of git-credential is that it will do
> that for you.
Absolutely.
> sub fill_credential {
> my $quoted_url = quotemeta(shift);
>
> my $verbatim = `git credential fill $quoted_url`;
> $? and die "git-credential failed";
>
> $verbatim =~ /^username=(.*)$/m
> or die "git-credential did not give us a username";
> my $username = $1;
> $verbatim =~ /^password=(.*)$/m
> or die "git-credential did not give us a password";
>
> return ($username, $password, $verbatim);
> }
>
> sub report_credential {
> my ($type, $verbatim) = @_;
> open(my $fh, '|-', "git credential $type");
> print $fh $verbatim;
> }
That sounds sensible too. We should be careful not to give a password as
argument (or users of the same machine will be able to find it with e.g.
"ps u"), but your proposal is OK with that.
>> + # error if key undef
>> + if (not defined $key) {
>> + print STDERR "ERROR reciving reponse git credential fill\n";
>> + exit 1;
>> + }
[...]
>> + } else {
>> + while (<Reader>) {
>> + print STDERR "\nERROR while running git credential $op:\n$_";
>> + }
>> + }
>> +}
>
> This isn't a good way to check for errors. The non-fill actions will
> never produce output on stdout, and you are not intercepting their
> stderr. Besides which, checking for errors by reading stderr is not a
> good practice; you should check the return value of the command in $?
> after it finishes.
I think it should do both. In case "git credential fill" returns
something that doesn't match the regexp, we don't want perl to error
with "use of undefined value", but that's just being defensive because
it shouldn't happen.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH_v1] add git credential login to remote mediawiki
2012-06-10 13:37 ` Matthieu Moy
@ 2012-06-10 17:36 ` roucherj
2012-06-10 18:39 ` Jeff King
1 sibling, 0 replies; 6+ messages in thread
From: roucherj @ 2012-06-10 17:36 UTC (permalink / raw)
To: Matthieu Moy
Cc: Jeff King, Javier.Roucher-Iglesias, git, Javier Roucher,
Pavel Volek, NGUYEN Kim Thuat, ROUCHER IGLESIAS Javier
On Sun, 10 Jun 2012 15:37:42 +0200, Matthieu Moy wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Sat, Jun 09, 2012 at 08:53:48PM +0200,
>> Javier.Roucher-Iglesias@ensimag.imag.fr wrote:
>>
>>> diff --git a/contrib/mw-to-git/git-remote-mediawiki
>>> b/contrib/mw-to-git/git-remote-mediawiki
>>> index c18bfa1..4b14d78 100755
>>> --- a/contrib/mw-to-git/git-remote-mediawiki
>>> +++ b/contrib/mw-to-git/git-remote-mediawiki
>>> @@ -152,28 +152,111 @@ while (<STDIN>) {
>>> ########################## Functions
>>> ##############################
>>>
>>> # MediaWiki API instance, created lazily.
>>> +sub run_credential {
>>
>> Is there any reason not to add this to perl/Git.pm? I suspect that
>> other
>> scripts will want to use it, too (for example, send-email could
>> probably
>> use it for SMTP credentials).
>
> Currently, git-remote-mediawiki is a standalone script (doesn't use
> Git.pm). This is good because it makes it trivial to install, but bad
> in
> the sense that it may force us (or others) to reinvent the wheel.
>
> Until now, the wheels we reinvented were very simple (run_git
> essentially), but we may be reaching the point where it makes sense
> to
> use and contribute to Git.pm.
>
> Unfortunately, from a non-technical point of view, Javier is
> contributing this as part of a student project, which ends this week,
> and it's probably not reasonable to introduce such change so late.
> So,
> I'd keep it here at least for now, and a move to Git.pm could be a
> separate future topic.
>
Thank's for explain my situation.
>>> + if (scalar(@_) == 2) {
>>> + if ($_[1] eq ("store" || "cache")) {
>>> + run_git("config credential.helper \'$_[1]\'");
>>> + } else {
>>> + print STDERR "ERROR: run_credential (fill|approve|reject)
>>> [store|cache]\n";
>>> + exit 1;
>>> + }
>>> + }
>>
>> This hunk looks wrong. You should never be setting the
>> credential.helper
>> config; that is the responsibility of the user to set, as they want
>> to
>> select whatever helper is appropriate. Nor do you need to care about
>> which helpers are in use; the point of git-credential is that it
>> will do
>> that for you.
>
> Absolutely.
>
I have add this with no advance warning, but i will remove it in the
next patch.
>> sub fill_credential {
>> my $quoted_url = quotemeta(shift);
>>
>> my $verbatim = `git credential fill $quoted_url`;
>> $? and die "git-credential failed";
>>
>> $verbatim =~ /^username=(.*)$/m
>> or die "git-credential did not give us a
>> username";
>> my $username = $1;
>> $verbatim =~ /^password=(.*)$/m
>> or die "git-credential did not give us a
>> password";
>>
>> return ($username, $password, $verbatim);
>> }
>>
>> sub report_credential {
>> my ($type, $verbatim) = @_;
>> open(my $fh, '|-', "git credential $type");
>> print $fh $verbatim;
>> }
>
> That sounds sensible too. We should be careful not to give a password
> as
> argument (or users of the same machine will be able to find it with
> e.g.
> "ps u"), but your proposal is OK with that.
>
>>> + # error if key undef
>>> + if (not defined $key) {
>>> + print STDERR "ERROR reciving reponse git credential fill\n";
>>> + exit 1;
>>> + }
> [...]
to be change, thanks for the corrections
>>> + } else {
>>> + while (<Reader>) {
>>> + print STDERR "\nERROR while running git credential $op:\n$_";
>>> + }
>>> + }
>>> +}
>>
>> This isn't a good way to check for errors. The non-fill actions will
>> never produce output on stdout, and you are not intercepting their
>> stderr. Besides which, checking for errors by reading stderr is not
>> a
>> good practice; you should check the return value of the command in
>> $?
>> after it finishes.
>
> I think it should do both. In case "git credential fill" returns
> something that doesn't match the regexp, we don't want perl to error
> with "use of undefined value", but that's just being defensive
> because
> it shouldn't happen.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH_v1] add git credential login to remote mediawiki
2012-06-10 13:37 ` Matthieu Moy
2012-06-10 17:36 ` roucherj
@ 2012-06-10 18:39 ` Jeff King
1 sibling, 0 replies; 6+ messages in thread
From: Jeff King @ 2012-06-10 18:39 UTC (permalink / raw)
To: Matthieu Moy
Cc: Javier.Roucher-Iglesias, git, Javier Roucher, Pavel Volek,
NGUYEN Kim Thuat, ROUCHER IGLESIAS Javier
On Sun, Jun 10, 2012 at 03:37:42PM +0200, Matthieu Moy wrote:
> > Is there any reason not to add this to perl/Git.pm? I suspect that other
> > scripts will want to use it, too (for example, send-email could probably
> > use it for SMTP credentials).
>
> Currently, git-remote-mediawiki is a standalone script (doesn't use
> Git.pm). This is good because it makes it trivial to install, but bad in
> the sense that it may force us (or others) to reinvent the wheel.
>
> Until now, the wheels we reinvented were very simple (run_git
> essentially), but we may be reaching the point where it makes sense to
> use and contribute to Git.pm.
Yeah, I noticed that. But hopefully since they are at least in the same
distribution, it is just a matter of getting the Makefiles right, and
will not be an extra burden on the user.
> Unfortunately, from a non-technical point of view, Javier is
> contributing this as part of a student project, which ends this week,
> and it's probably not reasonable to introduce such change so late. So,
> I'd keep it here at least for now, and a move to Git.pm could be a
> separate future topic.
Totally understood. I review from git's perspective, but it is up to you
to manage your students' workload. We can migrate the code to Git.pm
later (probably as part of a series which actually introduces a second
caller).
> > sub fill_credential {
> > my $quoted_url = quotemeta(shift);
> >
> > my $verbatim = `git credential fill $quoted_url`;
> > $? and die "git-credential failed";
> >
> > $verbatim =~ /^username=(.*)$/m
> > or die "git-credential did not give us a username";
> > my $username = $1;
> > $verbatim =~ /^password=(.*)$/m
> > or die "git-credential did not give us a password";
> >
> > return ($username, $password, $verbatim);
> > }
> >
> > sub report_credential {
> > my ($type, $verbatim) = @_;
> > open(my $fh, '|-', "git credential $type");
> > print $fh $verbatim;
> > }
>
> That sounds sensible too. We should be careful not to give a password as
> argument (or users of the same machine will be able to find it with e.g.
> "ps u"), but your proposal is OK with that.
Yes, that was intentional (and is the reason why helpers do so much over
stdin, even though it would reduce their parsing load if they could use
command-line arguments). Unfortunately, there is still one case that
reveals a password on the command-line: if a caller has a URL that
contains an embedded password like:
https://bob:secret@example.com/foo.git
It's tempting to say "well, then they don't need to ask git-credential
at all!". But the point of handing the whole URL to git-credential is so
that the caller doesn't _have_ to do the parsing. So how would it know
that the URL contains a password? :)
So instead of a URL on the command-line, it might make sense to simply
let the caller send an extra "url=" parameter on stdin (in addition to
any broken-down parameters, if it also wishes). It's way less convenient
(you are stuck with open2 from perl, rather than simple backticks), but
I think we should be cautious due to the security implications.
> >> + # error if key undef
> >> + if (not defined $key) {
> >> + print STDERR "ERROR reciving reponse git credential fill\n";
> >> + exit 1;
> >> + }
> [...]
> >> + } else {
> >> + while (<Reader>) {
> >> + print STDERR "\nERROR while running git credential $op:\n$_";
> >> + }
> >> + }
> >> +}
> >
> > This isn't a good way to check for errors. The non-fill actions will
> > never produce output on stdout, and you are not intercepting their
> > stderr. Besides which, checking for errors by reading stderr is not a
> > good practice; you should check the return value of the command in $?
> > after it finishes.
>
> I think it should do both. In case "git credential fill" returns
> something that doesn't match the regexp, we don't want perl to error
> with "use of undefined value", but that's just being defensive because
> it shouldn't happen.
Sorry, I just meant the latter block. It is checking for non-fill
actions to send any output, which they will never do (yes, it would be
an error for them to do so, but it is a very unlikely bug, and IMHO not
really worth checking).
-Peff
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-06-10 18:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-06-09 18:53 [PATCH_v1] add git credential login to remote mediawiki Javier.Roucher-Iglesias
2012-06-10 6:54 ` Junio C Hamano
2012-06-10 12:18 ` Jeff King
2012-06-10 13:37 ` Matthieu Moy
2012-06-10 17:36 ` roucherj
2012-06-10 18:39 ` Jeff King
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).