WIP: Trying to make LDAP login search subtrees for accounts to log into

app/Http/Controllers/Auth/LoginController.php

@@ -150,19 +150,20 @@ private function loginViaLdap(Request $request): User
              Log::debug("LDAP user ".$request->input('username')." not found in LDAP or could not bind");
              throw new \Exception("Could not find user in LDAP directory");
          } else {
-             Log::debug("LDAP user ".$request->input('username')." successfully bound to LDAP");
+             $username = $ldap_user[Setting::getSettings()->ldap_username_field][0]; // TODO - uh, I guess? I hate that we're doing this twice though
+             Log::debug("LDAP user $username successfully bound to LDAP");
          }
 
          // 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(); // FIXME - if we get more than one we should fail. and we sure about this ldap_import thing?
+         $user = User::where('username', '=', $username)->whereNull('deleted_at')->where('ldap_import', '=', 1)->where('activated', '=', '1')->first(); // FIXME - if we get more than one we should fail. and we sure about this ldap_import thing?
          Log::debug("Local auth lookup complete");
 
          // The user does not exist in the database. Try to get them from LDAP.
          // If user does not exist and authenticates successfully with LDAP we
          // will create it on the fly and sign in with default permissions
          if (!$user) {
-             Log::debug("Local user ".$request->input('username')." does not exist");
-             Log::debug("Creating local user ".$request->input('username'));
+             Log::debug("Local user ".$username." does not exist");
+             Log::debug("Creating local user ".username);
 
              if ($user = Ldap::createUserFromLdap($ldap_user)) { //this handles passwords on its own
                  Log::debug("Local user created.");
@@ -172,7 +173,7 @@ private function loginViaLdap(Request $request): User
              }
              // If the user exists and they were imported from LDAP already
          } else {
-             Log::debug("Local user ".$request->input('username')." exists in database. Updating existing user against LDAP.");
+             Log::debug("Local user ".$username." exists in database. Updating existing user against LDAP.");
 
              $ldap_attr = Ldap::parseAndMapLdapAttributes($ldap_user);
 

app/Http/Controllers/SettingsController.php

@@ -959,6 +959,7 @@ public function postLdapSettings(Request $request)
             $setting->ldap_dept = $request->input('ldap_dept');
             $setting->ldap_client_tls_cert   = $request->input('ldap_client_tls_cert');
             $setting->ldap_client_tls_key    = $request->input('ldap_client_tls_key');
+            $setting->ldap_append_basedn_to_username = $request->input('ldap_append_basedn_to_username','0');
 
 
         }

app/Models/Ldap.php

@@ -95,8 +95,50 @@ 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;
 
+        // FIND the user first, then attempt to log in as them using the appropriate attribute
+        $filterQuery = $settings->ldap_auth_filter_query.$username;
+        $filter = Setting::getSettings()->ldap_filter; //FIXME - this *does* respect the ldap filter, but I believe that AdLdap2 did *not*.
+        $filterQuery = "({$filter}({$filterQuery}))"; // FIXME - this makes an incorrect LDAP filter if there is no filter query
+
+        \Log::debug('Filter query: '.$filterQuery);
+
+
+        if (! self::bindAdminToLdap($connection) ) { //
+            \Log::debug("Uh, we could *NOT* bind as admin?! I don't understand how this ever worked.");
+            return false; // also FIXME? Maybe this one is OK?
+        }
+        \Log::debug("Well, I guess we at least bound as somebody?");
+        if (! $results = ldap_search($connection, $baseDn, $filterQuery)) {
+            throw new Exception('Could not search LDAP: '); // FIXME - I'm worried these early returns are somehow bad, in that they might disclose whether or not users exist?
+        }
+
+        if (! $entry = ldap_first_entry($connection, $results)) {
+            throw new Exception('Could not retrieve LDAP entry'); // TODO - should this throw an exception or just return false?
+            return false;
+        }
+
+        if (! $user = ldap_get_attributes($connection, $entry)) {
+            throw new Exception('Could not fetch LDAP attributes'); // TODO - should this throw an exception or just return false?
+            return false;
+        }
+
+        if ( ldap_next_entry($connection, $entry) !== false ) { //if there's a 'next' entry, then that's more than one - failure.
+            throw new Exception('Too many entries returned'); // (new) - TODO - should this throw an exception or just return false?
+            return false; //
+        }
+
+        \Log::debug("Entry retrieved was: ".print_r($user, true));
+        \Log::debug("The calculated LDAP username IS: ".print_r(array_change_key_case($user)[$ldap_username_field],true));
+
+        $username = array_change_key_case($user)[$ldap_username_field][0]; // TODO - is this right?
+        $userDn = $ldap_username_field.'='.$username;
+        if ( $settings->ldap_append_basedn_to_username ) { // TODO - we *could* actually disable/enable this field based on whether AD is on or not? (it will be ignored if AD is on, right?)
+            $userDn .= ','.$settings->ldap_basedn;
+        }
+        \Log::debug("initial username we think is: $userDn");
+        // *Now* we should try to bind as that found user
+        // first, AD has its own username-assembly system
         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
@@ -113,44 +155,11 @@ public static function findAndBindUserLdap($username, $password)
             }
         }
 
-        $filterQuery = $settings->ldap_auth_filter_query.$username;
-        $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;
-            }
-        }
-
-        if (! $results = ldap_search($connection, $baseDn, $filterQuery)) {
-            throw new Exception('Could not search LDAP: ');
-        }
-
-        if (! $entry = ldap_first_entry($connection, $results)) {
-            return false;
-        }
-
-        if (! $user = ldap_get_attributes($connection, $entry)) {
-            return false;
+            \Log::debug("Failed to log in to: $userDn with password '$password'");
+            throw new Exception("Unable to bind as user: $userDn");
         }
+        \Log::debug("Status of binding user: $userDn to directory: (directly!) ".($ldapbind ? "success" : "FAILURE"));
 
         return array_change_key_case($user);
     }
@@ -179,11 +188,7 @@ public static function bindAdminToLdap($connection)
         if (! $ldapbind = @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
-        // 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.
+        return $ldapbind; // will always be true - if it were false, we would've thrown an Exception
     }
 
 

database/migrations/2022_05_10_032248_add_append_base_d_n_checkbox_to_ldap_settings.php

@@ -0,0 +1,32 @@
+<?php
+
+use Illuminate\Database\Migrations\Migration;
+use Illuminate\Database\Schema\Blueprint;
+use Illuminate\Support\Facades\Schema;
+
+class AddAppendBaseDNCheckboxToLdapSettings extends Migration
+{
+    /**
+     * Run the migrations.
+     *
+     * @return void
+     */
+    public function up()
+    {
+        Schema::table('settings', function (Blueprint $table) {
+            $table->boolean('ldap_append_basedn_to_username')->default(1)->after('ldap_basedn');
+        });
+    }
+
+    /**
+     * Reverse the migrations.
+     *
+     * @return void
+     */
+    public function down()
+    {
+        Schema::table('settings', function (Blueprint $table) {
+            $table->dropColumn('ldap_append_basedn_to_username');
+        });
+    }
+}

resources/lang/en/admin/settings/general.php

@@ -99,6 +99,7 @@
     'ldap_country'              => 'LDAP Country',
     'ldap_pword'                => 'LDAP Bind Password',
     'ldap_basedn'               => 'Base Bind DN',
+    'ldap_append_basedn_to_username' => 'Append "," and Base DN to username',
     'ldap_filter'               => 'LDAP Filter',
     'ldap_pw_sync'              => 'LDAP Password Sync',
     'ldap_pw_sync_help'         => 'Uncheck this box if you do not wish to keep LDAP passwords synced with local passwords. Disabling this means that your users may not be able to login if your LDAP server is unreachable for some reason.',

resources/views/settings/ldap.blade.php

@@ -256,6 +256,20 @@
                             </div>
                         </div>
 
+                        <!-- LDAP append basedn to username -->
+                        <div class="form-group {{ $errors->has('ldap_basedn') ? 'error' : '' }}">
+                            <div class="col-md-3">
+                                {{ Form::label('ldap_append_basedn_to_username', trans('admin/settings/general.ldap_append_basedn_to_username')) }}
+                            </div>
+                            <div class="col-md-9">
+                                {{ Form::checkbox('ldap_append_basedn_to_username', '1', Request::old('ldap_append_basedn_to_username', $setting->ldap_append_basedn_to_username), [((config('app.lock_passwords')===true)) ? 'disabled ': '', 'class' => 'minimal '. $setting->demoMode, $setting->demoMode]) }}
+                                {!! $errors->first('ldap_append_basedn_to_username', '<span class="alert-msg" aria-hidden="true">:message</span>') !!}
+                                @if (config('app.lock_passwords')===true)
+                                    <p class="text-warning"><i class="fas fa-lock" aria-hidden="true"></i> {{ trans('general.feature_disabled') }}</p>
+                                @endif
+                            </div>
+                        </div>
+
                         <!-- LDAP filter -->
                         <div class="form-group {{ $errors->has('ldap_filter') ? 'error' : '' }}">
                             <div class="col-md-3">