Merge pull request #18058 from uberbrady/ldap_fetch_dn_before_login_FIXED

FIXED - perform Ldap fetch for dn (Distinguished Name) before logging-in (fixed)

app/Http/Controllers/Account/AcceptanceController.php

@@ -115,7 +115,8 @@ class AcceptanceController extends Controller
         }
 
         $item = $acceptance->checkoutable_type::find($acceptance->checkoutable_id);
-
+        $lastCheckout = $item->checkouts()->latest()->first();
+        $admin = $lastCheckout?->adminuser;
 
 
         // If signatures are required, make sure we have one
@@ -164,11 +165,10 @@ class AcceptanceController extends Controller
             'signature' => (($sig_filename && array_key_exists('1', $encoded_image))) ? $encoded_image[1] : null,
             'logo' => ($encoded_logo) ?? null,
             'date_settings' => $settings->date_display_format,
-            'admin' => auth()->user()->present()?->fullName,
+            'admin' => $admin?->present()->fullName,
             'qty' => $acceptance->qty ?? 1,
         ];
 
-
         if ($request->input('asset_acceptance') == 'accepted') {
 
 

app/Http/Controllers/Controller.php

@@ -45,6 +45,7 @@ abstract class Controller extends BaseController
         'accessories' => Accessory::class,
         'maintenances' => Maintenance::class,
         'assets' => Asset::class,
+        'audits' => Asset::class,
         'components' => Component::class,
         'consumables' => Consumable::class,
         'hardware' => Asset::class,
@@ -58,6 +59,7 @@ abstract class Controller extends BaseController
         'accessories' => 'private_uploads/accessories/',
         'maintenances' => 'private_uploads/maintenances/',
         'assets' => 'private_uploads/assets/',
+        'audits' => 'private_uploads/audits/',
         'components' => 'private_uploads/components/',
         'consumables' => 'private_uploads/consumables/',
         'hardware' => 'private_uploads/assets/',
@@ -71,6 +73,7 @@ abstract class Controller extends BaseController
         'accessories' => 'accessory',
         'maintenances' => 'maintenance',
         'assets' => 'asset',
+        'audits' => 'audits',
         'components' => 'component',
         'consumables' => 'consumable',
         'hardware' => 'asset',

app/Http/Transformers/ActionlogsTransformer.php

@@ -149,6 +149,7 @@ class ActionlogsTransformer
                     'filename' => $actionlog->filename,
                     'inlineable' => StorageHelper::allowSafeInline($actionlog->uploads_file_path()),
                     'exists_on_disk' => Storage::exists($actionlog->uploads_file_path()) ? true : false,
+                    'mediatype' => StorageHelper::getMediaType($actionlog->uploads_file_path()),
                 ] : null,
 
             'item' => ($actionlog->item) ? [

app/Models/Actionlog.php

@@ -478,6 +478,10 @@ class Actionlog extends SnipeModel
             $object = 'models';
         }
 
+        if ($this->action_type == 'audit') {
+            $object = 'audits';
+        }
+
         return route('ui.files.show', [
             'object_type' => $object,
             'id' => $this->item_id,

app/Models/Ldap.php

@@ -104,6 +104,78 @@ class Ldap extends Model
         return $connection;
     }
 
+    /**
+     * Finds user via Admin search *first*, and _then_ try to bind as that user, returning the user attributes on success,
+     * or false on failure. This enables login when the DN is harder to programmatically 'guess' due to having users in
+     * various different OU's or other LDAP entities.
+     */
+    public static function findAndBindMultiOU(string $baseDn, string $filterQuery, string $password, int $slow_failure = 3): array|false
+    {
+        /**
+         *  If you *don't* set the slow_failure variable, do note that we might permit timing attacks in here - if
+         *  your find results come back 'slow' when a user *does* exist, but fast if they *don't* exist, then you
+         *  can use this to enumerate users.
+         *
+         *  Even if that's *not* true, we still might have an issue: if we don't find the user, then we don't even _try_
+         *  to bind as them. Again, that could permit a timing attack.
+         *
+         *  Instead of checking every little thing, we just wrap everything in a try/catch in order to unify the
+         *  'slow_failure' treatment. All failures are re-raised as exceptions so that all failures exit from the
+         *  same place.
+         */
+        $connection = null;
+        $admin_conn = null;
+        try {
+            /**
+             * First we get an 'admin' connection, which will need search permissions. That was already a requirement
+             * here, so that's not a big lift. But it _is_ possible to configure LDAP to only login, and *not* to be
+             * able to import lists of users. In that case, this function *will not work* - and you should use the
+             * legacy 'findAndBindUserLdap' method, below. Otherwise, it looks like this would attempt an anonymous
+             * bind - which you might want, but you probably don't.
+             *
+             **/
+            $admin_conn = self::connectToLdap();
+            self::bindAdminToLdap($admin_conn);
+            $results = ldap_search($admin_conn, $baseDn, $filterQuery);
+            $entry_count = ldap_count_entries($admin_conn, $results);
+            if ($entry_count != 1) {
+                throw new \Exception('Wrong number of entries found: ' . $entry_count);
+            }
+            $entry = ldap_first_entry($admin_conn, $results);
+            $user = ldap_get_attributes($admin_conn, $entry);
+            $userDn = ldap_get_dn($admin_conn, $entry);
+            if (!$userDn) {
+                throw new \Exception("No user DN found");
+            }
+            \Log::debug("FOUND DN IS: $userDn");
+            // The temptation now is to do ldap_unbind on the $admin_conn, but that gets handled in the 'finally' below.
+            // I don't know if that means a separate 'connection' is maintained to the LDAP server or not, and would
+            // definitely prefer to not do that if we can avoid it. But I don't know enough about the LDAP protocol to
+            // be certain that that happens.
+
+            //now we try to log in (bind) as that found user
+            $connection = self::connectToLdap();
+            $bind_results = ldap_bind($connection, $userDn, $password);
+            if (!$bind_results) {
+                throw new \Exception("Unable to bind as user");
+            }
+            return array_change_key_case($user);
+        } catch (\Exception $e) {
+            \Log::debug("Exception on fast find-and-bind: " . $e->getMessage());
+            if ($slow_failure) {
+                sleep($slow_failure);
+            }
+            return false; //TODO - make this null instead for a slightly nicer type signature
+        } finally {
+            if ($admin_conn) {
+                ldap_unbind($admin_conn);
+            }
+            if ($connection) {
+                ldap_unbind($connection);
+            }
+        }
+    }
+
 
     /**
      * Binds/authenticates the user to LDAP, and returns their attributes.
@@ -147,6 +219,17 @@ class Ldap extends Model
 
         Log::debug('Filter query: '.$filterQuery);
 
+        // only try this if we have an Admin username set; otherwise use the 'legacy' method
+        if ($settings->ldap_uname) {
+            // in the fallowing call, we pick a slow-failure of 0 because we might need to fall through to 'legacy'
+            $fast_bind = self::findAndBindMultiOU($baseDn, $filterQuery, $password, 0);
+            if ($fast_bind) {
+                \Log::debug("Fast bind worked");
+                return $fast_bind;
+            }
+            \Log::debug("Fast bind failed; falling through to legacy bind");
+        }
+
         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)) {
@@ -154,11 +237,11 @@ class Ldap extends Model
                  * 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')
+                 * just "falls off the end" without ever explicitly 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!
+                 * If it *did* correctly return 'true' on a successful 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!!!!
                  *

resources/views/hardware/view.blade.php

@@ -1391,6 +1391,7 @@
                                         <th data-visible="true" data-field="icon" style="width: 40px;" class="hidden-xs" data-formatter="iconFormatter">{{ trans('admin/hardware/table.icon') }}</th>
                                         <th data-visible="true" data-field="created_at" data-sortable="true" data-formatter="dateDisplayFormatter">{{ trans('general.date') }}</th>
                                         <th data-visible="true" data-field="admin" data-formatter="usersLinkObjFormatter">{{ trans('general.created_by') }}</th>
+                                        <th data-visible="true" data-field="image" data-formatter="auditImageFormatter">{{ trans('general.image') }}</th>
                                         <th class="col-sm-2" data-field="file" data-sortable="true" data-visible="false" data-formatter="fileNameFormatter">{{ trans('general.file_name') }}</th>
                                         <th data-field="note">{{ trans('general.notes') }}</th>
                                         <th data-visible="false" data-field="file" data-visible="false"  data-formatter="fileDownloadButtonsFormatter">{{ trans('general.download') }}</th>

resources/views/partials/bootstrap-table.blade.php

@@ -1517,9 +1517,9 @@
         }
     }
 
-    function auditImageFormatter(value){
-        if (value){
-            return '<a href="' + value.url + '" data-toggle="lightbox" data-type="image"><img src="' + value.url + '" style="max-height: {{ $snipeSettings->thumbnail_max_h }}px; width: auto;" class="img-responsive" alt=""></a>'
+    function auditImageFormatter(value, row) {
+        if ((row) && (row.file) && (row.file.url)) {
+            return '<a href="' + row.file.url + '" data-toggle="lightbox" data-type="image"><img src="' + row.file.url + '" style="max-height: {{ $snipeSettings->thumbnail_max_h }}px; width: auto;" class="img-responsive" alt=""></a>'
         }
     }
 

routes/api.php

@@ -1348,7 +1348,7 @@ Route::group(['prefix' => 'v1', 'middleware' => ['api', 'api-throttle:api']], fu
             'index'
         ]
     )->name('api.files.index')
-        ->where(['object_type' => 'accessories|assets|components|consumables|hardware|licenses|locations|maintenances|models|users']);
+        ->where(['object_type' => 'accessories|audits|assets|components|consumables|hardware|licenses|locations|maintenances|models|users']);
 
     // Get a file
     Route::get('{object_type}/{id}/files/{file_id}',
@@ -1357,7 +1357,7 @@ Route::group(['prefix' => 'v1', 'middleware' => ['api', 'api-throttle:api']], fu
             'show'
         ]
     )->name('api.files.show')
-        ->where(['object_type' => 'accessories|assets|components|consumables|hardware|licenses|locations|maintenances|models|users']);
+        ->where(['object_type' => 'accessories|audits|assets|components|consumables|hardware|licenses|locations|maintenances|models|users']);
 
     // Upload files(s)
     Route::post('{object_type}/{id}/files',
@@ -1366,7 +1366,7 @@ Route::group(['prefix' => 'v1', 'middleware' => ['api', 'api-throttle:api']], fu
             'store'
         ]
     )->name('api.files.store')
-        ->where(['object_type' => 'accessories|assets|components|consumables|hardware|licenses|locations|maintenances|models|users']);
+        ->where(['object_type' => 'accessories|audits|assets|components|consumables|hardware|licenses|locations|maintenances|models|users']);
 
     // Delete files(s)
     Route::delete('{object_type}/{id}/files/{file_id}/delete',

routes/web.php

@@ -716,7 +716,7 @@ Route::group(['middleware' => 'web'], function () {
             'show'
         ]
     )->name('ui.files.show')
-        ->where(['object_type' => 'assets|maintenances|hardware|models|users|locations|accessories|consumables|licenses|components']);
+        ->where(['object_type' => 'assets|audits|maintenances|hardware|models|users|locations|accessories|consumables|licenses|components']);
 
     // Upload files(s)
     Route::post('{object_type}/{id}/files',
@@ -725,7 +725,7 @@ Route::group(['middleware' => 'web'], function () {
             'store'
         ]
     )->name('ui.files.store')
-        ->where(['object_type' => 'assets|maintenances|hardware|models|users|locations|accessories|consumables|licenses|components']);
+        ->where(['object_type' => 'assets|audits|maintenances|hardware|models|users|locations|accessories|consumables|licenses|components']);
 
     // Delete files(s)
     Route::delete('{object_type}/{id}/files/{file_id}/delete',

tests/Unit/LdapTest.php

@@ -81,18 +81,22 @@ class LdapTest extends TestCase
         $this->settings->enableLdap();
 
         $ldap_connect = $this->getFunctionMock("App\\Models", "ldap_connect");
-        $ldap_connect->expects($this->once())->willReturn('hello');
+        $ldap_connect->expects($this->exactly(3))->willReturn('hello');
 
         $ldap_set_option = $this->getFunctionMock("App\\Models", "ldap_set_option");
-        $ldap_set_option->expects($this->exactly(4));
+        $ldap_set_option->expects($this->exactly(12));
 
-        $this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->once())->willReturn(true);
+        $this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->exactly(2))->willReturn(true);
 
-        $this->getFunctionMock("App\\Models", "ldap_search")->expects($this->once())->willReturn(true);
+        $this->getFunctionMock("App\\Models", "ldap_search")->expects($this->exactly(1))->willReturn(true);
 
-        $this->getFunctionMock("App\\Models", "ldap_first_entry")->expects($this->once())->willReturn(true);
+        $this->getFunctionMock("App\\Models", "ldap_first_entry")->expects($this->exactly(1))->willReturn(true);
+        $this->getFunctionMock("App\\Models", "ldap_unbind")->expects($this->exactly(2));
+        $this->getFunctionMock("App\\Models", "ldap_count_entries")->expects($this->once())->willReturn(1);
+        $this->getFunctionMock("App\\Models", "ldap_get_dn")->expects($this->once())->willReturn('dn=FirstName Surname,ou=Org,dc=example,dc=com');
 
-        $this->getFunctionMock("App\\Models", "ldap_get_attributes")->expects($this->once())->willReturn(
+
+        $this->getFunctionMock("App\\Models", "ldap_get_attributes")->expects($this->exactly(1))->willReturn(
             [
                 "count" => 1,
                 0 => [
@@ -111,13 +115,25 @@ class LdapTest extends TestCase
         $this->settings->enableLdap();
 
         $ldap_connect = $this->getFunctionMock("App\\Models", "ldap_connect");
-        $ldap_connect->expects($this->once())->willReturn('hello');
+        $ldap_connect->expects($this->exactly(3))->willReturn('hello');
 
         $ldap_set_option = $this->getFunctionMock("App\\Models", "ldap_set_option");
-        $ldap_set_option->expects($this->exactly(4));
+        $ldap_set_option->expects($this->exactly(12));
 
-        // note - we return FALSE first, to simulate a bad-bind, then TRUE the second time to simulate a successful admin bind
-        $this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->exactly(2))->willReturn(false, true);
+        //
+        $this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->exactly(4))->willReturn(
+            true, /* initial admin connection for 'fast path' */
+            false, /* the actual login for the user */
+            false, /* the direct login for the user binding-as-themselves in the legacy path */
+            true /* the admin login afterwards (which is weird and doesn't make sense)  */
+        );
+        $this->getFunctionMock("App\\Models", "ldap_unbind")->expects($this->exactly(2));
+        $this->getFunctionMock("App\\Models", "ldap_error")->expects($this->never())->willReturn("exception");
+        $this->getFunctionMock("App\\Models", "ldap_search")->expects($this->exactly(1))->willReturn(false); //uhm?
+        $this->getFunctionMock("App\\Models", "ldap_count_entries")->expects($this->exactly(1))->willReturn(1);
+        $this->getFunctionMock("App\\Models", "ldap_first_entry")->expects($this->exactly(1))->willReturn(true);
+        $this->getFunctionMock("App\\Models", "ldap_get_attributes")->expects($this->exactly(1))->willReturn(1);
+        $this->getFunctionMock("App\\Models", "ldap_get_dn")->expects($this->exactly(1))->willReturn('dn=FirstName Surname,ou=Org,dc=example,dc=com');
 
 //        $this->getFunctionMock("App\\Models","ldap_error")->expects($this->once())->willReturn("exception");
 
@@ -132,14 +148,17 @@ class LdapTest extends TestCase
         $this->settings->enableLdap();
 
         $ldap_connect = $this->getFunctionMock("App\\Models", "ldap_connect");
-        $ldap_connect->expects($this->once())->willReturn('hello');
+        $ldap_connect->expects($this->exactly(2))->willReturn('hello');
 
         $ldap_set_option = $this->getFunctionMock("App\\Models", "ldap_set_option");
-        $ldap_set_option->expects($this->exactly(4));
+        $ldap_set_option->expects($this->exactly(8));
 
-        $this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->once())->willReturn(true);
+        $this->getFunctionMock("App\\Models", "ldap_bind")->expects($this->exactly(2))->willReturn(true); //I think this is OK
+
+        $this->getFunctionMock("App\\Models", "ldap_search")->expects($this->exactly(2))->willReturn(false); //uhm?
+        $this->getFunctionMock("App\\Models", "ldap_unbind")->expects($this->once());
+        $this->getFunctionMock("App\\Models", "ldap_count_entries")->expects($this->once())->willReturn(0);
 
-        $this->getFunctionMock("App\\Models", "ldap_search")->expects($this->once())->willReturn(false);
 
         $this->expectExceptionMessage("Could not search LDAP:");
         $results = Ldap::findAndBindUserLdap("username","password");