diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php
index 3f37efb2b5..341a004b67 100644
--- a/app/Http/Controllers/Auth/LoginController.php
+++ b/app/Http/Controllers/Auth/LoginController.php
@@ -145,7 +145,7 @@ class LoginController extends Controller
}
// Check if the user already exists in the database and was imported via LDAP
- $user = User::where('username', '=', $request->input('username'))->whereNull('deleted_at')->where('ldap_import', '=', 1)->where('activated', '=', '1')->first();
+ $user = User::where('username', '=', $request->input('username'))->whereNull('deleted_at')->where('ldap_import', '=', 1)->where('activated', '=', '1')->first(); // FIXME - if we get more than one we should fail.
Log::debug("Local auth lookup complete");
// The user does not exist in the database. Try to get them from LDAP.
diff --git a/app/Models/Ldap.php b/app/Models/Ldap.php
index b574820279..d7cf0732df 100644
--- a/app/Models/Ldap.php
+++ b/app/Models/Ldap.php
@@ -9,6 +9,22 @@ use Illuminate\Database\Eloquent\Model;
use Input;
use Log;
+/***********************************************
+ * TODOS:
+ *
+ * First off, we should probably make it so that the main LDAP thing we're using is an *instance* of this class,
+ * rather than the static methods we use here. We should probably load up that class with its settings, so we
+ * don't have to explicitly refer to them so often.
+ *
+ * Then, we should probably look at embedding some of the logic we use elsewhere into here - the various methods
+ * should either return a User or false, or other things like that. Don't make the consumers of this class reach
+ * into its guts. While that conflates this model with the User model, I think having the appropriate logic for
+ * turning LDAP people into Users ought to belong here, so it's easier on the consumer of this class.
+ *
+ * We're probably going to have to eventually make it so that Snipe-IT users can define multiple LDAP servers,
+ * and having this as a more instance-oriented class will be a step in the right direction.
+ ***********************************************/
+
class Ldap extends Model
{
/**
@@ -86,23 +102,41 @@ class Ldap extends Model
// If they are, we can skip building the UPN to authenticate against AD
if ($ldap_username_field == 'userprincipalname') {
$userDn = $username;
- } else { // FIXME - we have to respect the new 'append AD domain to username' setting (which sucks.)
- // In case they haven't added an AD domain
+ } 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;
}
}
$filterQuery = $settings->ldap_auth_filter_query.$username;
- $filter = Setting::getSettings()->ldap_filter; //TODO - this *does* respect the ldap filter, but I believe that AdLdap2 did *not*.
+ $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 uh, this seems...dangerous? Why would we just switch over to the admin connection? That's too loose, I feel.
- \Log::debug("Status of binding Admin user: $userDn to directory instead: ".($ldapbind ? "success" : "FAILURE"));
- return false;
+ if (! $ldapbind = self::bindAdminToLdap($connection)) {
+ /*
+ * FIXME 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;
}
}
@@ -135,8 +169,6 @@ class Ldap extends Model
{
$ldap_username = Setting::getSettings()->ldap_uname;
- $ldap_username = Setting::getSettings()->ldap_uname;
-
// Lets return some nicer messages for users who donked their app key, and disable LDAP
try {
$ldap_pass = \Crypt::decrypt(Setting::getSettings()->ldap_pword);
@@ -147,6 +179,11 @@ class Ldap extends Model
if (! $ldapbind = @ldap_bind($connection, $ldap_username, $ldap_pass)) {
throw new Exception('Could not bind to LDAP: '.ldap_error($connection));
}
+ // FIXME - this just "falls off the end" but the function states that it should return true or false
+ // unfortunately, one of the use cases for this function is wrong and *needs* for that failure mode to fire
+ // so I don't want to fix this right now.
+ // this method MODIFIES STATE on the passed-in $connection and just returns true or false (or, in this case, undefined)
+ // at the next refactor, this should be appropriately modified to be more consistent.
}
@@ -240,7 +277,7 @@ class Ldap extends Model
public static function findLdapUsers($base_dn = null, $count = -1)
{
$ldapconn = self::connectToLdap();
- $ldap_bind = self::bindAdminToLdap($ldapconn);
+ self::bindAdminToLdap($ldapconn);
// Default to global base DN if nothing else is provided.
if (is_null($base_dn)) {
$base_dn = Setting::getSettings()->ldap_basedn;
diff --git a/resources/views/settings/ldap.blade.php b/resources/views/settings/ldap.blade.php
index e01ad2c86a..e87a432da2 100644
--- a/resources/views/settings/ldap.blade.php
+++ b/resources/views/settings/ldap.blade.php
@@ -122,7 +122,7 @@
-
+ {{-- NOTICE - this was a feature for AdLdap2-based LDAP syncing, and is already handled in 'classic' LDAP, so we now hide the checkbox (but haven't deleted the field)