Bug 18947: LDAP - do not assume anonymous bind if no user or password

To test:
Ideally tested on a working ldap server with bind by auth and no
anonymous bind
1  - Define an LDAP config with bind by auth
2  - Don't define user/pass
3  - Define anonymous_bind = 0
4  - Attempt bind by auth
5  - Error is something like:
LDAP search failed to return object : XXXXXXXXX: LdapErr: XXXX-XXXXXX,
     comment: In order to perform this operation a successful bind must
     be completed on the connection., data 0, v2580 at
     /usr/share/koha/lib/C4/Auth_with_ldap.pm line 102.
6  - Define user/pass
7  - Now bind by auth should work
8  - remove user/pass
9  - Apply patch
10 - Attempt again
11 - Bind by auth shoudl succeed

prove -v t/db_dependent/Auth_with_ldap.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

diff --git a/C4/Auth_with_ldap.pm b/C4/Auth_with_ldap.pm
index 53f0eb6..b5876a0 100644 (file)
--- a/C4/Auth_with_ldap.pm
+++ b/C4/Auth_with_ldap.pm
@@ -59,7 +59,6 @@ my $prefhost  = $ldap->{hostname}     or die ldapserver_error('hostname');
 my $base      = $ldap->{base}          or die ldapserver_error('base');
 $ldapname     = $ldap->{user}          ;
 $ldappassword = $ldap->{pass}          ;
-$ldap->{anonymous_bind} = 1 unless $ldapname && $ldappassword;
 our %mapping  = %{$ldap->{mapping}}; # FIXME dpavlin -- don't die because of || (); from 6eaf8511c70eb82d797c941ef528f4310a15e9f9
 my @mapkeys = keys %mapping;
 $debug and print STDERR "Got ", scalar(@mapkeys), " ldap mapkeys (  total  ): ", join ' ', @mapkeys, "\n";
@@ -76,7 +75,7 @@ if(defined $ldap->{categorycode_mapping}) {
 }
 
 my %config = (
-       anonymous => ($ldapname and $ldappassword) ? 0 : 1,
+    anonymous => defined ($ldap->{anonymous_bind}) ? $ldap->{anonymous_bind} : 1,
     replicate => defined($ldap->{replicate}) ? $ldap->{replicate} : 1,  #    add from LDAP to Koha database for new user
        update => defined($ldap->{update}   ) ? $ldap->{update}    : 1,  # update from LDAP to Koha database for existing user
 );
@@ -127,7 +126,7 @@ sub checkpw_ldap {
     # first, LDAP authentication
     if ( $ldap->{auth_by_bind} ) {
         my $principal_name;
-        if ( $ldap->{anonymous_bind} ) {
+        if ( $config{anonymous} ) {
 
             # Perform an anonymous bind
             my $res = $db->bind;
@@ -155,7 +154,7 @@ sub checkpw_ldap {
         # Perform a LDAP bind for the given username using the matched DN
         my $res = $db->bind( $principal_name, password => $password );
         if ( $res->code ) {
-            if ( $ldap->{anonymous_bind} ) {
+            if ( $config{anonymous} ) {
                 # With anonymous_bind approach we can be sure we have found the correct user
                 # and that any 'code' response indicates a 'bad' user (be that blocked, banned
                 # or password changed). We should not fall back to local accounts in this case.
@@ -176,7 +175,7 @@ sub checkpw_ldap {
             $userldapentry = $search->shift_entry;
         }
     } else {
-               my $res = ($ldap->{anonymous_bind}) ? $db->bind : $db->bind($ldapname, password=>$ldappassword);
+        my $res = ($config{anonymous}) ? $db->bind : $db->bind($ldapname, password=>$ldappassword);
                if ($res->code) {               # connection refused
                        warn "LDAP bind failed as ldapuser " . ($ldapname || '[ANONYMOUS]') . ": " . description($res);
                        return 0;

diff --git a/t/db_dependent/Auth_with_ldap.t b/t/db_dependent/Auth_with_ldap.t
index 1a1c3d0..e0c3c45 100755 (executable)
--- a/t/db_dependent/Auth_with_ldap.t
+++ b/t/db_dependent/Auth_with_ldap.t
@@ -41,6 +41,8 @@ my $update         = 0;
 my $replicate      = 0;
 my $auth_by_bind   = 1;
 my $anonymous_bind = 1;
+my $user           = 'cn=Manager,dc=metavore,dc=com';
+my $pass           = 'metavore';
 
 # Variables controlling LDAP behaviour
 my $desired_authentication_result = 'success';
@@ -143,7 +145,7 @@ subtest 'checkpw_ldap tests' => sub {
 
     subtest 'auth_by_bind = 1 tests' => sub {
 
-        plan tests => 9;
+        plan tests => 11;
 
         $auth_by_bind = 1;
 
@@ -161,6 +163,19 @@ subtest 'checkpw_ldap tests' => sub {
           'checkpw_ldap prints correct warning if LDAP anonymous bind fails';
         is( $ret, 0, 'checkpw_ldap returns 0 if LDAP anonymous bind fails' );
 
+        $anonymous_bind = 0;
+        $user = undef;
+        $pass = undef;
+        reload_ldap_module();
+
+        warning_like {
+            $ret = C4::Auth_with_ldap::checkpw_ldap( $dbh, 'hola',
+                password => 'hey' );
+        }
+        qr/LDAP bind failed as kohauser hola: LDAP error #1: error_name/,
+          'checkpw_ldap prints correct warning if LDAP bind_by_auth fails';
+        is( $ret, 0, 'checkpw_ldap returns 0 if LDAP bind_by_auth fails' );
+
         $desired_authentication_result = 'success';
         $anonymous_bind                = 1;
         $desired_admin_bind_result   = 'success';
@@ -251,8 +266,10 @@ subtest 'checkpw_ldap tests' => sub {
 
         # Anonymous bind
         $anonymous_bind            = 1;
+        $user                      = 'cn=Manager,dc=metavore,dc=com';
+        $pass                      = 'metavore';
         $desired_admin_bind_result = 'error';
-        $desired_bind_result = 'error';
+        $desired_bind_result       = 'error';
         reload_ldap_module();
 
         warning_like {
@@ -361,11 +378,11 @@ sub mockedC4Config {
             base           => 'dc=metavore,dc=com',
             hostname       => 'localhost',
             mapping        => \%ldap_mapping,
-            pass           => 'metavore',
+            pass           => $pass,
             principal_name => '%s@my_domain.com',
             replicate      => $replicate,
             update         => $update,
-            user           => 'cn=Manager,dc=metavore,dc=com',
+            user           => $user,
         );
         return \%ldap_config;
     }