Better documentation, disable AdLdap2-based "Add domain" setting

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.

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;

resources/views/settings/ldap.blade.php

@@ -122,7 +122,7 @@
                             </div>
                         </div><!-- AD Domain -->
 
-                        <!-- AD Append Domain -->
+                        {{-- 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) <!-- AD Append Domain -->
                         <div class="form-group">
                             <div class="col-md-3">
                                 {{ Form::label('ad_append_domain', trans('admin/settings/general.ad_append_domain_label')) }}
@@ -136,7 +136,7 @@
                                     <p class="text-warning"><i class="fas fa-lock" aria-hidden="true"></i> {{ trans('general.feature_disabled') }}</p>
                                 @endif
                             </div>
-                        </div>
+                        </div> --}}
 
                         <!-- LDAP Client-Side TLS key -->
                         <div class="form-group {{ $errors->has('ldap_client_tls_key') ? 'error' : '' }}">