On Sun, Jul 4, 2010 at 08:51, Áshin László <ashinlaszlo@xxxxxxxxx> wrote:
> Hi,
>
> On Sun, Jul 4, 2010 at 00:14, Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
>> On Sat, Jul 3, 2010 at 21:21, Áshin László <ashinlaszlo@xxxxxxxxx> wrote:
>>> Documentation/git-cvsserver.txt | 46 +++++++++++++++++++++++++++++++++++---
>>> git-cvsserver.perl | 28 +++++++++++++++++++++++
>>> t/t9400-git-cvsserver-server.sh | 45 ++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 115 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt
>>> index 7004dd2..b3c3122 100644
>>> --- a/Documentation/git-cvsserver.txt
>>> +++ b/Documentation/git-cvsserver.txt
>>> @@ -100,10 +100,48 @@ looks like
>>> ------
>>>
>>> Only anonymous access is provided by pserve by default. To commit you
>>
>> I think it's always called "pserver" with an -r.
>
> Yes but it is not part of my changeset.
Sorry, misread the diff.
>>> -will have to create pserver accounts, simply add a gitcvs.authdb
>>> -setting in the config file of the repositories you want the cvsserver
>>> -to allow writes to, for example:
>>> +will have to specify an authentication option in the config file.
>>> +Currently there are two options are available for authentication through
>>> +pserver in git-cvsserver: one through an authenticator script and an other
>>> +through a textual authentication database. If both are specified in the config
>>> +file then the script based solution will be used.
>>
>> If both are specified shouldn't we throw an error?
>
> Actually a check can be added to the code. OK, I will do that.
Nice.
>>> + a. To use the authentication script based method, simply add a
>>> + gitcvs.authscript setting in the config file of the repositories you want
>>> + the cvsserver to allow writes to, for example:
>>> ++
>>> +--
>>> +------
>>> +
>>> + [gitcvs]
>>> + authscript = /usr/local/bin/cvsserver-auth.sh
>>> +
>>> +------
>>> +The file specified here must be executable by the user the git-cvsserver runs
>>> +under the name of. This script has to read exactly two lines from its standard
>>
>> "the name of" is redundant here.
>>
>>> +input as long as git-cvsserver passes the username and the password on separate
>>> +lines. After the script did its task by checking the username and password
>>> +given it has to return zero if the authentication was successful and return
>>> +other value if it was not.
>>
>> Better as: "The script will receive two lines on standard input, the
>> first is the username and the second is the password. It should return
>> 0 if the user was successfully authenticated, and a non-zero value if
>> not".
>
> OK, a little bit I overcomplicated it :)
>
>
>>> +
>>> +Here is an example for an authentication script which checks the users against
>>> +active directory:
>>> +------
>>> +#!/bin/sh
>>> +# /usr/local/bin/cvsserver-auth.sh
>>>
>>> +read username
>>> +read password
>>> +
>>> +wbinfo -a "${username}%${password}"
>>
>> Do all POSIX shells implicitly exit with the return value of the last
>> statement they evaluate. I don't know.
>
> "The exit status of a sequential list *shall* be the exit status of
> the last command in the list."
> ( After registration,
> http://www.opengroup.org/onlinepubs/000095399/utilities/xcu_chap02.html#tag_02_08
> )
> Generally, all kind of list constructions use this principle according
> to this page.
>
> Terminology of "shall":
> "For an implementation that conforms to IEEE Std 1003.1-2001,
> describes a feature or behavior that is mandatory. *An application can
> rely on the existence of the feature or behavior*.
>
> For an application or user, describes a behavior that is mandatory."
Nice, good to have that cleared up in general :)
>>> +------
>>> +--
>>> +
>>> + b. To use the authentication database based method, simply add a
>>> + gitcvs.authdb setting in the config file of the repositories you want the
>>> + cvsserver to allow writes to, for example:
>>> ++
>>> +--
>>> ------
>>>
>>> [gitcvs]
>>> @@ -125,7 +163,7 @@ Alternatively you can produce the password with
>>> perl's crypt() operator:
>>> -----
>>> perl -e 'my ($user, $pass) = @ARGV; printf "%s:%s\n", $user,
>>> crypt($user, $pass)' $USER password
>>> -----
>>> -
>>> +--
>>> Then provide your password via the pserver method, for example:
>>> ------
>>> cvs -d:pserver:someuser:somepassword <at> server/path/repo.git co <HEAD_name>
>>> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
>>> index e9f3037..9664e86 100755
>>> --- a/git-cvsserver.perl
>>> +++ b/git-cvsserver.perl
>>> @@ -197,6 +197,34 @@ if ($state->{method} eq 'pserver') {
>>> }
>>>
>>> # Fall through to LOVE
>>> + } elsif (exists $cfg->{gitcvs}->{authscript}) {
>>> + my $authscript = $cfg->{gitcvs}->{authscript};
>>> +
>>> + unless (-x $authscript) {
>>> + print "E The authentication script specified in ";
>>> + print "[gitcvs.authscript] cannot be executed\n";
>>
>> I *think* you have to prefix anything that's not "I (HATE|LOVE) YOU"
>> with "E " if it's an error. I.e. this should probably be:
>>
>> print "E The authentication script specified in";
>> print "E [gitcvs.authscript] cannot be executed\n";
>>
>> But that's just my hazy memory. Maybe CVS clients will report the
>> error anyway.
>
> Please note that there is no "\n" after the first line. This is only a
> line break in the source code.
> Should I have used the following?
Ah yes, I missed that there wasn't a \n there.
> print "E The authentication script specified in" .
> "E [gitcvs.authscript] cannot be executed\n";
Probably yeah, at least I managed to misread it :)
>>> + print "I HATE YOU\n";
>>> + exit 1;
>>> + }
>>> +
>>> + open my $script_fd, '|-', "'$authscript'"
>>> + or die "Couldn't open authentication script '$authscript': $!";
>>> +
>>> + if (length($password) > 0) {
>>> + $password = descramble($password);
>>> + }
>>> +
>>> + print $script_fd "$user\n";
>>> + print $script_fd "$password\n";
>>> + close $script_fd;
>>> +
>>> + unless ($? == 0) {
>>> + print "E External script authentication failed.\n";
>>> + print "I HATE YOU\n";
>>> + exit 1;
>>> + }
>>> +
>>> + # Fall through to LOVE
>>> } else {
>>> # Trying to authenticate a user
>>> if (not exists $cfg->{gitcvs}->{authdb}) {
>>> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
>>> index 8639506..0743e9a 100755
>>> --- a/t/t9400-git-cvsserver-server.sh
>>> +++ b/t/t9400-git-cvsserver-server.sh
>>> @@ -64,6 +64,16 @@ test_expect_success 'basic checkout' \
>>> # PSERVER AUTHENTICATION
>>> #------------------------
>>>
>>> +cat >"$SERVERDIR/authscript.sh" <<EOF
>>> +#!/bin/sh
>>> +read username
>>> +read password
>>> +
>>> +test "x\$username" = xcvsuser -a "x\$password" = xcvspassword
>>
>> This is offtopic. But I've never figured out why you need to do
>>
>> test "x$foo" = "xbar"
>>
>> As opposed to:
>>
>> test "$foo" = "bar"
>>
>> In shellscript. Are there really some shells that get equality tests
>> where one term is "" wrong?
>
> Maybe this is too much. Maybe x was used only where " couldn't be:
>
> test x$foo = xbar
>
> So, if $foo is empty we have to make sure that test gets something to
> both side of the equation mark. But if quotation marks can be used it
> is possibly unneeded:
>
> test "$foo" = "bar"
>
> My solution is an overkill in this case. But which of the above ones
> is preferred then?
> I think I can answer my own question. If $foo contains spaces only the
> quotation mark based version will do what we want. I will remove the x
> characters.
Ok.
>>> +EOF
>>> +
>>> +chmod a+x "$SERVERDIR/authscript.sh"
>>> +
>>> cat >request-anonymous <<EOF
>>> BEGIN AUTH REQUEST
>>> $SERVERDIR
>>> @@ -134,6 +144,41 @@ test_expect_success 'pserver authentication
>>> failure (login/non-anonymous user)'
>>> fi &&
>>> sed -ne \$p log | grep "^I HATE YOU\$"'
>>>
>>> +GIT_DIR="$SERVERDIR" git config --unset gitcvs.authdb || exit 1
>>> +GIT_DIR="$SERVERDIR" git config gitcvs.authscript
>>> "$SERVERDIR/authscript.sh" || exit 1
>>> +
>>> +test_expect_success 'pserver authentication (authscript)' \
>>> + 'cat request-anonymous | git-cvsserver pserver >log 2>&1 &&
>>> + sed -ne \$p log | grep "^I LOVE YOU\$"'
>>> +
>>> +test_expect_success 'pserver authentication failure (authscript,
>>> non-anonymous user)' \
>>> + 'if cat request-git | git-cvsserver pserver >log 2>&1
>>> + then
>>> + false
>>> + else
>>> + true
>>> + fi &&
>>
>> This should probably be (untested):
>>
>> test_must_fail cat request-git git-cvsserver pserver >log 2>&1 &&
>
> I copied this code from above so this was not my decision. These test
> cases are all can be find elsewhere in the code, the only difference
> is in their names and the fact that the script based authentication
> method is active while they are running.
Yeah, a lot of the test code is ancient and unoptimal. Just leave it
like that I guess.
>>> + sed -ne \$p log | grep "^I HATE YOU\$"'
>>> +
>>> +test_expect_success 'pserver authentication success (authscript,
>>> non-anonymous user with password)' \
>>> + 'cat login-git-ok | git-cvsserver pserver >log 2>&1 &&
>>> + sed -ne \$p log | grep "^I LOVE YOU\$"'
>>> +
>>> +test_expect_success 'pserver authentication (authscript, login)' \
>>> + 'cat login-anonymous | git-cvsserver pserver >log 2>&1 &&
>>> + sed -ne \$p log | grep "^I LOVE YOU\$"'
>>> +
>>> +test_expect_success 'pserver authentication failure (authscript,
>>> login/non-anonymous user)' \
>>> + 'if cat login-git | git-cvsserver pserver >log 2>&1
>>> + then
>>> + false
>>> + else
>>> + true
>>> + fi &&
>>> + sed -ne \$p log | grep "^I HATE YOU\$"'
>>> +
>>> +GIT_DIR="$SERVERDIR" git config --unset gitcvs.authscript || exit 1
>>> +GIT_DIR="$SERVERDIR" git config gitcvs.authdb "$SERVERDIR/auth.db" || exit 1
>>>
>>> # misuse pserver authentication for testing of req_Root
>>
>> Otherwise looking good.
>>
>
> I am not familiar the way that a patch goes in this project. Where can
> I find the repository my patches will be commited? When will this kind
> of changes go to mainline?
Patches are:
* Discussed on the mailing list (you are here)
* Submitted for inclusion (see Documentation/SubmittingPatches). You
didn't CC Junio so it isn't there yet.
* Maybe show up in a "What's cooking" post if they're more complex
and warrant further discussion.
* Get merged into git://git.kernel.org/pub/scm/git/git.git, either
pu, next, master, maint or some combo of that.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html