Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #15926 : wrong LDAP service account login behavior #15927

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+18 −27
Open

Fixed #15926 : wrong LDAP service account login behavior #15927

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 18 additions & 27 deletions app/Models/Ldap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public static function findAndBindUserLdap($username, $password)
$connection = self::connectToLdap();
$ldap_username_field = $settings->ldap_username_field;
$baseDn = $settings->ldap_basedn;
$userDn = $ldap_username_field.'='.$username.','.$settings->ldap_basedn;
$userDn = null;

if ($settings->is_ad == '1') {
// Check if they are using the userprincipalname for the username field.
Expand All @@ -119,39 +119,30 @@ public static function findAndBindUserLdap($username, $password)

Log::debug('Filter query: '.$filterQuery);

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;
}
}

if (! $results = ldap_search($connection, $baseDn, $filterQuery)) {
throw new Exception('Could not search LDAP: ');
// Find user in LDAP
$findResult = self::findLdapUsers(filter: $filterQuery);
if ($findResult['count'] != 1) {
Log::debug("No or multiple users found for query $filterQuery. baseDn: $baseDn");
return false;
}

if (! $entry = ldap_first_entry($connection, $results)) {

// Ensure found user ldap attributes match the required attributes
if (! $findResult[0][$ldap_username_field] == $username) {
Log::debug("Username from LDAP does not match the username provided.");
return false;
}

if (! $user = ldap_get_attributes($connection, $entry)) {
// Once user found, bind to LDAP
if ($userDn === null) {
$userDn = $findResult[0]['dn'];
}
if (! $ldapbind = @ldap_bind($connection, $userDn, $password)) {
Log::debug("Could not bind to LDAP as $userDn");
return false;
}

$user = $findResult[0];

return array_change_key_case($user);
}

Expand Down