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

Jump to bottom

Fix LDAP login for dn which is not uid=${username},${base} #14137

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix LDAP login for dn which is not uid=${username},${base} #14137

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
62 changes: 17 additions & 45 deletions app/Models/Ldap.php
View file
Open in desktop
Expand Up @@ -93,65 +93,37 @@ public static function findAndBindUserLdap($username, $password)
{
$settings = Setting::getSettings();
$connection = self::connectToLdap();
$ldap_username_field = $settings->ldap_username_field;
$baseDn = $settings->ldap_basedn;
$userDn = $ldap_username_field.'='.$username.','.$settings->ldap_basedn;

if ($settings->is_ad == '1') {
// Check if they are using the userprincipalname for the username field.
// If they are, we can skip building the UPN to authenticate against AD
if ($ldap_username_field == 'userprincipalname') {
$userDn = $username;
} else {
// TODO - we no longer respect the "add AD Domain to username" checkbox, but it still exists in settings.
// We should probably just eliminate that checkbox to avoid confusion.
// We let it sit in the DB, unused, to facilitate people downgrading (if they decide to).
// Hopefully, in a later release, we can remove it from the settings.
// This logic instead just means that if we're using UPN, we don't append ad_domain, if we aren't, then we do.
// Hopefully that should handle all of our use cases, but if not we can backport our old logic.
$userDn = ($settings->ad_domain != '') ? $username.'@'.$settings->ad_domain : $username.'@'.$settings->email_domain;
}
}
self::bindAdminToLdap($connection);

// FIND the user first, then attempt to log in as them using the appropriate attribute
$baseDn = $settings->ldap_basedn;
$filterQuery = $settings->ldap_auth_filter_query.$username;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of concatenating the $username, I think it would be better to provide a string template in the config and then inserting the username into the template, this would allow ldap_auth_filter_query like:

| (uid=$username) (mail=$username)

which would enable people to loin with either uid or email

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would also enable using only a single config field for what is now "ldap_filter" and "ldap_auth_filter_query"

$filter = Setting::getSettings()->ldap_filter; //FIXME - this *does* respect the ldap filter, but I believe that AdLdap2 did *not*.
$filterQuery = "({$filter}({$filterQuery}))";

\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;
}
}
$filter = $settings->ldap_filter;
$filterQuery = "(& ({$filter}) ({$filterQuery}))";

if (! $results = ldap_search($connection, $baseDn, $filterQuery)) {
throw new Exception('Could not search LDAP: ');
}

if (! $entry = ldap_first_entry($connection, $results)) {
// Empty results -> no user matched query
\Log::debug("No user matched LDAP query");
return false;
}

if (! $user = ldap_get_attributes($connection, $entry)) {
if (! $userDn = ldap_get_dn($connection, $entry)) {
throw new Exception("Could not get ldap dn for entry");
}

if (! $ldapbind = @ldap_bind($connection, $userDn, $password)) {
\Log::warning("LDAP bind did not succeed for $username with dn $userDn");
return false;
}

if (! $user = ldap_get_attributes($connection, $entry)) {
throw new Exception("Could not user");
}

return array_change_key_case($user);
}

Expand Down