From b7bd56daf722909a59682f4f1c9ece6301b71ebf Mon Sep 17 00:00:00 2001 From: Brady Wetherington Date: Thu, 13 Feb 2025 15:09:28 +0000 Subject: [PATCH] Instead of saving TLS cache-files on save, cache them when used --- app/Http/Controllers/SettingsController.php | 1 - app/Models/Setting.php | 61 ++++++++++----------- tests/Unit/LdapTest.php | 27 +++++++++ 3 files changed, 57 insertions(+), 32 deletions(-) diff --git a/app/Http/Controllers/SettingsController.php b/app/Http/Controllers/SettingsController.php index 1bc120d1c..0b15f3562 100755 --- a/app/Http/Controllers/SettingsController.php +++ b/app/Http/Controllers/SettingsController.php @@ -869,7 +869,6 @@ class SettingsController extends Controller } if ($setting->save()) { - $setting->update_client_side_cert_files(); return redirect()->route('settings.ldap.index') ->with('success', trans('admin/settings/message.update.success')); } diff --git a/app/Models/Setting.php b/app/Models/Setting.php index 232285fbd..8f8299a3c 100755 --- a/app/Models/Setting.php +++ b/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,6 +340,33 @@ class Setting extends Model 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 * @@ -346,7 +374,7 @@ class Setting extends Model */ 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 @@ class Setting extends Model */ 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()); - } - } - } - - } diff --git a/tests/Unit/LdapTest.php b/tests/Unit/LdapTest.php index f3bc52163..a492b1efb 100644 --- a/tests/Unit/LdapTest.php +++ b/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 @@ class LdapTest extends TestCase $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'); + } + }