Added an error message bag for connection issues with LDAP

app/Http/Controllers/Auth/LoginController.php

@@ -302,14 +302,17 @@ public function login(Request $request)
         if (Setting::getSettings()->ldap_enabled) { // avoid hitting the $this->ldap
             LOG::debug('LDAP is enabled.');
             try {
-                LOG::debug('Attempting to log user in by LDAP authentication.');
                 $user = $this->loginViaLdap($request);
                 Auth::login($user, $request->input('remember'));
-
-                // If the user was unable to login via LDAP, log the error and let them fall through to
-            // local authentication.
             } catch (\Exception $e) {
-                Log::debug('There was an error authenticating the LDAP user: '.$e->getMessage());
+
+                Session::flash('error', $e->getMessage());
+                Log::warning("LDAP bind failed ({$e}");
+                return back()->withInput();
+            } catch (\Throwable $e) {
+                Session::flash('error', 'Login failed. Please try again.');
+                Log::error($e);
+                return back()->withInput();
             }
         }
 

app/Models/Ldap.php

@@ -97,7 +97,13 @@ public static function connectToLdap()
         ldap_set_option($connection, LDAP_OPT_NETWORK_TIMEOUT, 20);
 
         if ($ldap_use_tls=='1') {
-            ldap_start_tls($connection);
+            //suppresses the error and throws exception.
+            if (! @ldap_start_tls($connection)) {
+                $code = ldap_errno($connection);
+                $err  = ldap_error($connection);
+
+                throw new \Exception("Could not start TLS with LDAP (code $code): $err.");
+            }
         }
 
 
@@ -108,14 +114,15 @@ public static function connectToLdap()
     /**
      * Binds/authenticates the user to LDAP, and returns their attributes.
      *
-     * @author [A. Gianotto] [<snipe@snipe.net>]
-     * @since  [v3.0]
      * @param  $username
      * @param  $password
-     * @param  bool|false $user
+     * @param bool|false $user
      * @return bool true    if the username and/or password provided are valid
      *              false   if the username and/or password provided are invalid
      *         array of ldap_attributes if $user is true
+     * @throws Exception
+     * @since  [v3.0]
+     * @author [A. Gianotto] [<snipe@snipe.net>]
      */
     public static function findAndBindUserLdap($username, $password)
     {
@@ -147,29 +154,45 @@ public static function findAndBindUserLdap($username, $password)
 
         Log::debug('Filter query: '.$filterQuery);
 
+        //Suppressing the error and handling it to be more friendly
         if (! $ldapbind = @ldap_bind($connection, $userDn, $password)) {
-            Log::debug("Status of binding user: $userDn to directory: (directly!) ".($ldapbind ? "success" : "FAILURE"));
-            if (! $ldapbind = self::bindAdminToLdap($connection)) {
-                /*
-                 * TODO PLEASE:
-                 *
-                 * this isn't very clear, so it's important to note: the $ldapbind value is never correctly returned - we never 'return true' from self::bindAdminToLdap() (the function
-                 * just "falls off the end" without ever explictly returning 'true')
-                 *
-                 * but it *does* have an interesting side-effect of checking for the LDAP password being incorrectly encrypted with the wrong APP_KEY, so I'm leaving it in for now.
-                 *
-                 * If it *did* correctly return 'true' on a succesful bind, it would _probably_ allow users to log in with an incorrect password. Which would be horrible!
-                 *
-                 * Let's definitely fix this at the next refactor!!!!
-                 *
-                 */
-                Log::debug("Status of binding Admin user: $userDn to directory instead: ".($ldapbind ? "success" : "FAILURE"));
-                return false;
-            }
+            $code = ldap_errno($connection);
+            $err  = ldap_error($connection);
+
+            Log::warning("LDAP bind FAILED for DN={$userDn} code={$code} error={$err}");
+
+            //More codes can be found under Client side result codes at ldap.com
+            $friendly = trans('auth/message.account_not_found');
+
+            throw new Exception(
+                $friendly,
+                $code,
+            );
         }
+            Log::debug("Status of binding user: $userDn to directory: (directly!) ".($ldapbind ? "success" : "FAILURE"));
+//            if (! $ldapbind = self::bindAdminToLdap($connection)) {
+//                /*
+//                 * TODO PLEASE:
+//                 *
+//                 * this isn't very clear, so it's important to note: the $ldapbind value is never correctly returned - we never 'return true' from self::bindAdminToLdap() (the function
+//                 * just "falls off the end" without ever explictly returning 'true')
+//                 *
+//                 * but it *does* have an interesting side-effect of checking for the LDAP password being incorrectly encrypted with the wrong APP_KEY, so I'm leaving it in for now.
+//                 *
+//                 * If it *did* correctly return 'true' on a succesful bind, it would _probably_ allow users to log in with an incorrect password. Which would be horrible!
+//                 *
+//                 * Let's definitely fix this at the next refactor!!!!
+//                 *
+//                 */
+//                Log::debug("Status of binding Admin user: $userDn to directory instead: ".($ldapbind ? "success" : "FAILURE"));
+//                return false;
+//            }
+
+        // BindAdminToLdap throws on failure now, and continues on success is the above block still needed? This could also be done by adding return true to the bindadmintoldap method, but Ill leave this here for now.
+        self::bindAdminToLdap($connection);
 
         if (! $results = ldap_search($connection, $baseDn, $filterQuery)) {
-            throw new Exception('Could not search LDAP: ');
+            return false;
         }
 
         if (! $entry = ldap_first_entry($connection, $results)) {
@@ -205,7 +228,7 @@ public static function bindAdminToLdap($connection)
                 throw new Exception('Your app key has changed! Could not decrypt LDAP password using your current app key, so LDAP authentication has been disabled. Login with a local account, update the LDAP password and re-enable it in Admin > Settings.');
             }
 
-            if (! $ldapbind = @ldap_bind($connection, $ldap_username, $ldap_pass)) {
+            if (!@ldap_bind($connection, $ldap_username, $ldap_pass)) {
                 throw new Exception('Could not bind to LDAP: '.ldap_error($connection));
             }
             // TODO - this just "falls off the end" but the function states that it should return true or false
@@ -215,7 +238,7 @@ public static function bindAdminToLdap($connection)
             // at the next refactor, this should be appropriately modified to be more consistent.
         } else {
             // LDAP should also work with anonymous bind (no dn, no password available)
-            if (! $ldapbind = @ldap_bind($connection)) {
+            if (!@ldap_bind($connection)) {
                 throw new Exception('Could not bind to LDAP: '.ldap_error($connection));
             }
         }

tests/Unit/LdapTest.php

@@ -86,7 +86,9 @@ public function testFindAndBind()
         $ldap_set_option = $this->getFunctionMock("App\\Models", "ldap_set_option");
         $ldap_set_option->expects($this->exactly(4));
 
-        $this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->once())->willReturn(true);
+        $this->getFunctionMock("App\\Models", "ldap_bind")
+            ->expects($this->exactly(2))
+            ->willReturnOnConsecutiveCalls(true, true);
 
         $this->getFunctionMock("App\\Models", "ldap_search")->expects($this->once())->willReturn(true);
 
@@ -116,34 +118,56 @@ public function testFindAndBindBadPassword()
         $ldap_set_option = $this->getFunctionMock("App\\Models", "ldap_set_option");
         $ldap_set_option->expects($this->exactly(4));
 
-        // note - we return FALSE first, to simulate a bad-bind, then TRUE the second time to simulate a successful admin bind
-        $this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->exactly(2))->willReturn(false, true);
-
+        $this->getFunctionMock('App\\Models', 'ldap_bind')
+            ->expects($this->once())
+            ->willReturn(false);
+        $this->getFunctionMock('App\\Models', 'ldap_errno')
+            ->expects($this->once())
+            ->willReturn(49); // typical "invalid creds"
+        $this->getFunctionMock('App\\Models', 'ldap_error')
+            ->expects($this->once())
+            ->willReturn('Invalid credentials');
 //        $this->getFunctionMock("App\\Models","ldap_error")->expects($this->once())->willReturn("exception");
 
 
 //        $this->expectExceptionMessage("exception");
-        $results = Ldap::findAndBindUserLdap("username","password");
-        $this->assertFalse($results);
+        try {
+            Ldap::findAndBindUserLdap('username', 'password');
+            $this->fail('Expected exception not thrown');
+        } catch (\Exception $e) {
+            $this->assertSame(49, $e->getCode());
+        }
     }
 
+    /**
+     * @throws \Exception
+     */
     public function testFindAndBindCannotFindSelf()
     {
         $this->settings->enableLdap();
 
         $ldap_connect = $this->getFunctionMock("App\\Models", "ldap_connect");
+
         $ldap_connect->expects($this->once())->willReturn('hello');
 
         $ldap_set_option = $this->getFunctionMock("App\\Models", "ldap_set_option");
         $ldap_set_option->expects($this->exactly(4));
 
-        $this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->once())->willReturn(true);
+        $this->getFunctionMock('App\\Models', 'ldap_bind')
+            ->expects($this->exactly(2))
+            ->willReturnOnConsecutiveCalls(true, true);
 
-        $this->getFunctionMock("App\\Models", "ldap_search")->expects($this->once())->willReturn(false);
+        $this->getFunctionMock('App\\Models', 'ldap_search')
+            ->expects($this->once())
+            ->willReturn(false);
 
-        $this->expectExceptionMessage("Could not search LDAP:");
-        $results = Ldap::findAndBindUserLdap("username","password");
-        $this->assertFalse($results);
+        $this->getFunctionMock('App\\Models', 'ldap_first_entry')->expects($this->never());
+        $this->getFunctionMock('App\\Models', 'ldap_get_attributes')->expects($this->never());
+
+        $this->getFunctionMock("App\\Models", "ldap_errno")->expects($this->never());
+        $this->getFunctionMock('App\\Models', 'ldap_error')->expects($this->never());
+
+        $this->assertFalse(Ldap::findAndBindUserLdap('username', 'password'));
     }
 
     //maybe should do an AD test as well?
@@ -153,6 +177,8 @@ public function testFindLdapUsers()
         $this->settings->enableLdap();
 
         $ldap_connect = $this->getFunctionMock("App\\Models", "ldap_connect");
+        $this->getFunctionMock("App\\Models", "ldap_errno")->expects($this->never());
+        $this->getFunctionMock("App\\Models", "ldap_error")->expects($this->never());
         $ldap_connect->expects($this->once())->willReturn('hello');
 
         $ldap_set_option = $this->getFunctionMock("App\\Models", "ldap_set_option");
@@ -165,7 +191,6 @@ public function testFindLdapUsers()
         $this->getFunctionMock("App\\Models", "ldap_parse_result")->expects($this->once())->willReturn(true);
 
         $this->getFunctionMock("App\\Models", "ldap_get_entries")->expects($this->once())->willReturn(["count" => 1]);
-
         $results = Ldap::findLdapUsers();
 
         $this->assertEqualsCanonicalizing(["count" => 1], $results);