441503 - Need update on contrib/syncLDAP.pl

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[A. Shimono [:himorin]]

Attachment: patch for syncLDAP.pl

current contrib/syncLDAP.pl causes an error when we use uri schema on LDAPserver parameter. (It's ok with normal localhost:389 type, but fails on ldap://localhost:389)

I think it might be better to patch this script.

Comment 1

[Emmanuel Seyman]

In contrib/syncLDAP, we have :

my $LDAPport = "389";  # default LDAP port
if($LDAPserver =~ /:/) {
    ($LDAPserver, $LDAPport) = split(":",$LDAPserver);
}

If you submit an URI, you get $LDAPserver which contains "ldap" and $LDAPport which contains "localhost:389" (or whatever your ldap server is set to in params).

Comment 2

[Emmanuel Seyman]

After reading the documentation for Net::LDAP, I'm not sure separating the LDAPserver param into host and port parts is necessary.

You can just pass $LDAPserver as host, "389" as port to Net::LDAP's new function and things work fine.

Comment 3

[A. Shimono [:himorin]]

(In reply to comment #2)
> After reading the documentation for Net::LDAP, I'm not sure separating the
> LDAPserver param into host and port parts is necessary.

Sorry, i forgot about that. You're definitely true.
So, problem is not from uri scheme but the port number tweak in syncLDAP.pl code.
i'll update my patch.

Comment 4

[A. Shimono [:himorin]]

Attachment: patch v2 for syncLDAP.pl

update

Comment 5

[Emmanuel Seyman]

With the second version of the patch, anyone who uses just a hostname (ie. localhost) as $LDAPserver will not be able to connect.

Again, I'm pretty sure that you only need to remove these three lines from the syncLDAP script :

if($LDAPserver =~ /:/) {
    ($LDAPserver, $LDAPport) = split(":",$LDAPserver);
}

and everything should work fine.

Comment 6

[A. Shimono [:himorin]]

(In reply to comment #5)
> Again, I'm pretty sure that you only need to remove these three lines from the
> syncLDAP script :

i've tried once, but failed with ldaps:// uri. removing port setting, it works.
# It seems against to the Net::LDAP documentation,,,

Comment 7

[Emmanuel Seyman]

(In reply to comment #6)
>
> i've tried once, but failed with ldaps:// uri. removing port setting, it works.

This shouldn't happen. Net::LDAP contains this line of code :
  $host =~ s/^([^:]+|\[.*\]):(\d+)$/$1/ and $port = $2;

so it should be doing your job for you.
I'll see if I can test this today.

Comment 8

[Frédéric Buclin]

himorin, ask manu specifically for review so that he sees this patch in his review queue (or manu: feel free to set the review flag after testing).

Comment 9

[A. Shimono [:himorin]]

i've tested @
$ perl -MNet::LDAP -e 'print Net::LDAP->VERSION'
0.34

Comment 10

[Emmanuel Seyman]

Comment on attachment 326506 [details] [diff] [review]
patch v2 for syncLDAP.pl


>+if($LDAPserver =~ /\:\/\//) {

Nitpick: ':' isn't a special caracter in regexps and there's no need to escape it.
This could be fixed on checkin, I guess.
I've put this patch through the paces and it checks out fine.
r+ from me.

Comment 11

[Frédéric Buclin]

Could you (either manu or himorin) request approval for affected branches?

Comment 12

[A. Shimono [:himorin]]

target code block of this exists on file version 1.1 (@cvs)

Comment 13

[Frédéric Buclin]

2.x are restricted to security fixes.

Comment 14

[Emmanuel Seyman]

3.0:

Checking in contrib/syncLDAP.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/syncLDAP.pl,v  <--  syncLDAP.pl
new revision: 1.9.2.2; previous revision: 1.9.2.1
done

3.2:

Checking in contrib/syncLDAP.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/syncLDAP.pl,v  <--  syncLDAP.pl
new revision: 1.12.2.1; previous revision: 1.12
done

tip:

Checking in contrib/syncLDAP.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/syncLDAP.pl,v  <--  syncLDAP.pl
new revision: 1.13; previous revision: 1.12
done

Comment 15

[Emmanuel Seyman]

And, of course, I forgot to correct the nitpick before committing.

3.0:

Checking in contrib/syncLDAP.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/syncLDAP.pl,v  <--  syncLDAP.pl
new revision: 1.9.2.3; previous revision: 1.9.2.2
done

3.2:

Checking in contrib/syncLDAP.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/syncLDAP.pl,v  <--  syncLDAP.pl
new revision: 1.12.2.2; previous revision: 1.12.2.1
done

tip:

Checking in contrib/syncLDAP.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/syncLDAP.pl,v  <--  syncLDAP.pl
new revision: 1.14; previous revision: 1.13
done