From ed09674e9034b471d2ecacd2787c8417a3701c62 Mon Sep 17 00:00:00 2001 From: Godfrey M Date: Thu, 18 Sep 2025 12:50:25 -0700 Subject: [PATCH 1/8] adds an error message bag for connection issues with LDAP --- app/Http/Controllers/Auth/LoginController.php | 15 +++++--- app/Models/Ldap.php | 37 ++++++++++++++++--- 2 files changed, 41 insertions(+), 11 deletions(-) diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php index 0b8786545514..013fd616aaea 100644 --- a/app/Http/Controllers/Auth/LoginController.php +++ b/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. + \Auth::login($user); } catch (\Exception $e) { - Log::debug('There was an error authenticating the LDAP user: '.$e->getMessage()); + + Session::flash('error', $e->getMessage()); + Log::notice("LDAP bind failed ({$e}"); + return back()->withInput(); + } catch (\Throwable $e) { + Session::flash('error', 'Login failed. Please try again.'); + Log::error($e); + return back()->withInput(); } } diff --git a/app/Models/Ldap.php b/app/Models/Ldap.php index 168da8b57170..e9a5b0b4c847 100644 --- a/app/Models/Ldap.php +++ b/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 grabs it. + 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] [] - * @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] [] */ public static function findAndBindUserLdap($username, $password) { @@ -147,7 +154,28 @@ 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)) { + $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 = match ($code) { + 49 => 'Invalid username or password.', + 34 => 'Invalid username format for LDAP.', + 81 => 'Cannot reach the LDAP server. Please try again later.', + 85 => 'LDAP request timed out. Please try again later.', + 52 => 'LDAP service unavailable. Please try again later.', + default => 'Could not contact LDAP server. Please try again.', + }; + + throw new Exception( + $friendly, + $code, + ); + } Log::debug("Status of binding user: $userDn to directory: (directly!) ".($ldapbind ? "success" : "FAILURE")); if (! $ldapbind = self::bindAdminToLdap($connection)) { /* @@ -166,7 +194,6 @@ public static function findAndBindUserLdap($username, $password) Log::debug("Status of binding Admin user: $userDn to directory instead: ".($ldapbind ? "success" : "FAILURE")); return false; } - } if (! $results = ldap_search($connection, $baseDn, $filterQuery)) { throw new Exception('Could not search LDAP: '); From 109331bc3f9c7dbd3d93e80ef2b24d66edb456c6 Mon Sep 17 00:00:00 2001 From: Godfrey M Date: Mon, 22 Sep 2025 12:03:20 -0700 Subject: [PATCH 2/8] remove some unnecessary messages --- app/Models/Ldap.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/Models/Ldap.php b/app/Models/Ldap.php index e9a5b0b4c847..0b6f223587e8 100644 --- a/app/Models/Ldap.php +++ b/app/Models/Ldap.php @@ -97,7 +97,7 @@ public static function connectToLdap() ldap_set_option($connection, LDAP_OPT_NETWORK_TIMEOUT, 20); if ($ldap_use_tls=='1') { - //suppresses the error and grabs it. + //suppresses the error and throws exception. if (! @ldap_start_tls($connection)) { $code = ldap_errno($connection); $err = ldap_error($connection); @@ -163,12 +163,10 @@ public static function findAndBindUserLdap($username, $password) //More codes can be found under Client side result codes at ldap.com $friendly = match ($code) { - 49 => 'Invalid username or password.', - 34 => 'Invalid username format for LDAP.', 81 => 'Cannot reach the LDAP server. Please try again later.', 85 => 'LDAP request timed out. Please try again later.', 52 => 'LDAP service unavailable. Please try again later.', - default => 'Could not contact LDAP server. Please try again.', + default => 'Could not contact LDAP server. Please try again. Hello', }; throw new Exception( From 8c7e431ead3e87822d825699533232e0132a3dda Mon Sep 17 00:00:00 2001 From: Godfrey M Date: Thu, 25 Sep 2025 10:56:49 -0700 Subject: [PATCH 3/8] re added a code --- app/Models/Ldap.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/Models/Ldap.php b/app/Models/Ldap.php index 0b6f223587e8..8e3205517612 100644 --- a/app/Models/Ldap.php +++ b/app/Models/Ldap.php @@ -163,10 +163,11 @@ public static function findAndBindUserLdap($username, $password) //More codes can be found under Client side result codes at ldap.com $friendly = match ($code) { + 49 => 'Invalid username or password.', 81 => 'Cannot reach the LDAP server. Please try again later.', 85 => 'LDAP request timed out. Please try again later.', 52 => 'LDAP service unavailable. Please try again later.', - default => 'Could not contact LDAP server. Please try again. Hello', + default => 'Could not contact LDAP server. Please try again.', }; throw new Exception( From bb940135b390de6e4b89ab780e9a2ec0d58380fd Mon Sep 17 00:00:00 2001 From: Godfrey M Date: Tue, 30 Sep 2025 11:13:32 -0700 Subject: [PATCH 4/8] use a default message --- app/Models/Ldap.php | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/app/Models/Ldap.php b/app/Models/Ldap.php index 8e3205517612..321e916fb583 100644 --- a/app/Models/Ldap.php +++ b/app/Models/Ldap.php @@ -162,13 +162,7 @@ public static function findAndBindUserLdap($username, $password) 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 = match ($code) { - 49 => 'Invalid username or password.', - 81 => 'Cannot reach the LDAP server. Please try again later.', - 85 => 'LDAP request timed out. Please try again later.', - 52 => 'LDAP service unavailable. Please try again later.', - default => 'Could not contact LDAP server. Please try again.', - }; + $friendly = 'Invalid username or password.'; throw new Exception( $friendly, From 3c4a392e498d0ec505fa1c3af24f4253c2294f4c Mon Sep 17 00:00:00 2001 From: Godfrey M Date: Tue, 30 Sep 2025 11:15:51 -0700 Subject: [PATCH 5/8] readd remember user --- app/Http/Controllers/Auth/LoginController.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php index 013fd616aaea..65a3cc6009dd 100644 --- a/app/Http/Controllers/Auth/LoginController.php +++ b/app/Http/Controllers/Auth/LoginController.php @@ -303,11 +303,11 @@ public function login(Request $request) LOG::debug('LDAP is enabled.'); try { $user = $this->loginViaLdap($request); - \Auth::login($user); + Auth::login($user, $request->input('remember')); } catch (\Exception $e) { Session::flash('error', $e->getMessage()); - Log::notice("LDAP bind failed ({$e}"); + Log::warning("LDAP bind failed ({$e}"); return back()->withInput(); } catch (\Throwable $e) { Session::flash('error', 'Login failed. Please try again.'); From 14111907a857b3629b77426f83ffa859fd60887c Mon Sep 17 00:00:00 2001 From: Godfrey M Date: Wed, 1 Oct 2025 10:40:59 -0700 Subject: [PATCH 6/8] use translation --- app/Models/Ldap.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Models/Ldap.php b/app/Models/Ldap.php index 321e916fb583..b17d0664c7e0 100644 --- a/app/Models/Ldap.php +++ b/app/Models/Ldap.php @@ -162,7 +162,7 @@ public static function findAndBindUserLdap($username, $password) 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 = 'Invalid username or password.'; + $friendly = trans('auth/message.account_not_found'); throw new Exception( $friendly, From 8d7d4f44fc7ac9d2d35922a49eb885f326d35cad Mon Sep 17 00:00:00 2001 From: Godfrey M Date: Tue, 7 Oct 2025 12:51:24 -0700 Subject: [PATCH 7/8] fix tests and disable code block because throws been added --- app/Models/Ldap.php | 43 ++++++++++++++++++++++------------------- tests/Unit/LdapTest.php | 25 ++++++++++++++++++------ 2 files changed, 42 insertions(+), 26 deletions(-) diff --git a/app/Models/Ldap.php b/app/Models/Ldap.php index b17d0664c7e0..578f18712ce7 100644 --- a/app/Models/Ldap.php +++ b/app/Models/Ldap.php @@ -170,26 +170,29 @@ public static function findAndBindUserLdap($username, $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; - } +// 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)) { @@ -225,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 @@ -235,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)); } } diff --git a/tests/Unit/LdapTest.php b/tests/Unit/LdapTest.php index 3f0116097683..f38e8fe962d3 100644 --- a/tests/Unit/LdapTest.php +++ b/tests/Unit/LdapTest.php @@ -127,23 +127,35 @@ public function testFindAndBindBadPassword() $this->assertFalse($results); } + /** + * @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 +165,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 +179,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); From e256b4636df41111de219853eadbb4a251fd030d Mon Sep 17 00:00:00 2001 From: Godfrey M Date: Tue, 7 Oct 2025 13:17:40 -0700 Subject: [PATCH 8/8] fix other tests --- tests/Unit/LdapTest.php | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/Unit/LdapTest.php b/tests/Unit/LdapTest.php index f38e8fe962d3..577e3bac51eb 100644 --- a/tests/Unit/LdapTest.php +++ b/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,15 +118,25 @@ 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()); + } } /**