Merge pull request #16250 from uberbrady/improve_tls_client_side_file_caching

Instead of saving TLS cache-files on save, cache them when used

Author: snipe

app/Http/Controllers/SettingsController.php

@@ -869,7 +869,6 @@ public function postLdapSettings(StoreLdapSettings $request) : RedirectResponse
         }
 
         if ($setting->save()) {
-            $setting->update_client_side_cert_files();
             return redirect()->route('settings.ldap.index')
                 ->with('success', trans('admin/settings/message.update.success'));
         }

app/Models/Setting.php

@@ -2,6 +2,7 @@
 
 namespace App\Models;
 
+use Carbon\Carbon;
 use Illuminate\Database\Eloquent\Factories\HasFactory;
 use Illuminate\Database\Eloquent\Model;
 use Illuminate\Notifications\Notifiable;
@@ -339,14 +340,41 @@ public static function getLdapSettings(): Collection
         return collect($ldapSettings);
     }
 
+    /**
+     * For a particular cache-file, refresh it if the settings have
+     * been updated more recently than the file. Then return the
+     * full filepath
+     */
+    public static function get_fresh_file_path($attribute, $path)
+    {
+        $full_path = storage_path().'/'.$path;
+        $file_exists = file_exists($full_path);
+        if ($file_exists) {
+            $statblock = stat($full_path);
+        }
+        if (!$file_exists || Carbon::createFromTimestamp($statblock['mtime']) < Setting::getSettings()->updated_at) {
+            if (Setting::getSettings()->{$attribute}) {
+                file_put_contents($full_path, Setting::getSettings()->{$attribute});
+            } else {
+                //this doesn't fire when you might expect it to because a lot of the time we do something like:
+                // if ($settings->ldap_client_tls_cert && ...
+                // so we never get a chance to 'uncache' the file.
+                if ($file_exists) {
+                    unlink($full_path);
+                }
+            }
+        }
+        return $full_path;
+    }
+
     /**
      * Return the filename for the client-side SSL cert
      *
      * @var string
      */
     public static function get_client_side_cert_path()
     {
-        return storage_path().'/ldap_client_tls.cert';
+        return self::get_fresh_file_path('ldap_client_tls_cert', 'ldap_client_tls.cert');
     }
 
     /**
@@ -356,36 +384,7 @@ public static function get_client_side_cert_path()
      */
     public static function get_client_side_key_path()
     {
-        return storage_path().'/ldap_client_tls.key';
+        return self::get_fresh_file_path('ldap_client_tls_key', 'ldap_client_tls.key');
     }
 
-    public function update_client_side_cert_files()
-    {
-        /**
-         * I'm not sure if it makes sense to have a cert but no key
-         * nor vice versa, but for now I'm just leaving it like this.
-         *
-         * Also, we could easily set this up with an event handler and
-         * self::saved() or something like that but there's literally only
-         * one place where we will do that, so I'll just explicitly call
-         * this method at that spot instead. It'll be easier to debug and understand.
-         */
-        if ($this->ldap_client_tls_cert) {
-            file_put_contents(self::get_client_side_cert_path(), $this->ldap_client_tls_cert);
-        } else {
-            if (file_exists(self::get_client_side_cert_path())) {
-                unlink(self::get_client_side_cert_path());
-            }
-        }
-
-        if ($this->ldap_client_tls_key) {
-            file_put_contents(self::get_client_side_key_path(), $this->ldap_client_tls_key);
-        } else {
-            if (file_exists(self::get_client_side_key_path())) {
-                unlink(self::get_client_side_key_path());
-            }
-        }
-    }
-
-
 }

tests/Unit/LdapTest.php

@@ -2,6 +2,7 @@
 
 namespace Tests\Unit;
 
+use App\Models\Setting;
 use PHPUnit\Framework\Attributes\Group;
 use App\Models\Ldap;
 use Tests\TestCase;
@@ -206,4 +207,30 @@ function ($ldapconn, $search_results, $errcode , $matcheddn , $errmsg , $referra
         $this->assertEqualsCanonicalizing(["count" => 2], $results);
     }
 
+    public function testNonexistentTLSFile()
+    {
+        $this->settings->enableLdap()->set(['ldap_client_tls_cert' => 'SAMPLE CERT TEXT']);
+        $certfile = Setting::get_client_side_cert_path();
+        $this->assertStringEqualsFile($certfile, 'SAMPLE CERT TEXT');
+    }
+
+    public function testStaleTLSFile()
+    {
+        file_put_contents(Setting::get_client_side_cert_path(), 'STALE CERT FILE');
+        sleep(1); // FIXME - this is going to slow down tests
+        $this->settings->enableLdap()->set(['ldap_client_tls_cert' => 'SAMPLE CERT TEXT']);
+        $certfile = Setting::get_client_side_cert_path();
+        $this->assertStringEqualsFile($certfile, 'SAMPLE CERT TEXT');
+    }
+
+    public function testFreshTLSFile()
+    {
+        $this->settings->enableLdap()->set(['ldap_client_tls_cert' => 'SAMPLE CERT TEXT']);
+        $client_side_cert_path = Setting::get_client_side_cert_path();
+        file_put_contents($client_side_cert_path, 'WEIRDLY UPDATED CERT FILE');
+        //the system should respect our cache-file, since the settings haven't been updated
+        $possibly_recached_cert_file = Setting::get_client_side_cert_path(); //this should *NOT* re-cache from the Settings
+        $this->assertStringEqualsFile($possibly_recached_cert_file, 'WEIRDLY UPDATED CERT FILE');
+    }
+
 }