fix(manager): stop stale page locks, alert loops and double alerts

core/src/Core.php

@@ -3786,7 +3786,9 @@ function __alertQuit() {
                         el.remove();
                         " . $fnc . "
                     }
-                    window.setTimeout('__alertQuit();',100);
+                    // after load: a manager tab or popup frame replaces alert() in its own onload; a timer
+                    // started earlier fires first on a slow stylesheet, and the message shows twice
+                    window.addEventListener('load', function () { window.setTimeout(__alertQuit, 100); });
                 </script>
             </head>
             <body>
@@ -3935,13 +3937,7 @@ public function buildLockedElementsCache()
     public function cleanupExpiredLocks()
     {
         // Clean-up active_user_sessions first
-        if ((int) $this->getConfig('session_timeout') < 2) {
-            $timeout = 120;
-        } else {
-            $timeout = $this->getConfig('session_timeout') * 60;
-        }
-        // session.js pings every 10min, so 2min is minimum
-        $validSessionTimeLimit = $this->time - $timeout;
+        $validSessionTimeLimit = ActiveUserSession::validSince((int) $this->time, (int) $this->getConfig('session_timeout'));
         ActiveUserSession::where('lasthit', '<', (int) $validSessionTimeLimit)->delete();
 
         // Clean-up active_user_locks

core/src/Models/ActiveUser.php

@@ -12,7 +12,7 @@
  * @property string $action
  * @property int $id
  *
- * @method static \Illuminate\Database\Eloquent\Builder|\EvolutionCMS\Models\ActiveUser locked($action, $id = null, $userId = null)
+ * @method static \Illuminate\Database\Eloquent\Builder|\EvolutionCMS\Models\ActiveUser locked($action, $id = null, $userId = null, ?int $aliveSince = null)
  *
  * @mixin \Eloquent
  */
@@ -38,14 +38,20 @@ class ActiveUser extends Eloquent\Model
         'id'
     ];
 
-    public function scopeLocked(Eloquent\Builder $builder, $action, $id = null, $userId = null)
+    public function scopeLocked(Eloquent\Builder $builder, $action, $id = null, $userId = null, ?int $aliveSince = null)
     {
         if ($userId === null) {
             $userId = evo()->getLoginUserID();
         }
+        if ($aliveSince === null) {
+            $aliveSince = ActiveUserSession::validSince(time(), (int) evo()->getConfig('session_timeout'));
+        }
 
+        // A row stays here after its manager is gone (closed browser, expired session) until they open
+        // another page, so only a manager who is still signed in holds the page.
         $builder = $builder->where('action', '=', (int)$action)
             ->where('internalKey', '!=', $userId)
+            ->whereIn('internalKey', ActiveUserSession::query()->select('internalKey')->where('lasthit', '>=', $aliveSince))
             ->orderBy('lasthit', 'DESC');
 
         if ($id !== null) {

core/src/Models/ActiveUserSession.php

@@ -31,4 +31,13 @@ class ActiveUserSession extends Eloquent\Model
         'lasthit',
         'ip'
     ];
+
+    /**
+     * Oldest lasthit of a session that still counts as signed in: session_timeout minutes, at least 2.
+     * An open manager pings every half timeout (evo.keepMeAlive), so it never falls out of this window.
+     */
+    public static function validSince(int $now, int $timeoutMinutes): int
+    {
+        return $now - max(2, $timeoutMinutes) * 60;
+    }
 }

core/tests/Unit/Manager/SettingsLockTest.php

@@ -0,0 +1,82 @@
+<?php
+
+use EvolutionCMS\Models\ActiveUser;
+use EvolutionCMS\Models\ActiveUserSession;
+use Illuminate\Database\Capsule\Manager as Capsule;
+use Illuminate\Database\Eloquent\Model;
+use Illuminate\Database\Schema\Blueprint;
+
+beforeEach(function () {
+    $capsule = new Capsule();
+    $capsule->addConnection(['driver' => 'sqlite', 'database' => ':memory:', 'prefix' => '']);
+    $capsule->setAsGlobal();
+    $capsule->bootEloquent();
+    Model::setConnectionResolver($capsule->getDatabaseManager());
+
+    $schema = $capsule->getConnection()->getSchemaBuilder();
+    $schema->create('active_user_sessions', function (Blueprint $table) {
+        $table->string('sid', 128)->primary();
+        $table->integer('internalKey')->default(0);
+        $table->integer('lasthit')->default(0);
+        $table->string('ip')->default('');
+    });
+    $schema->create('active_users', function (Blueprint $table) {
+        $table->string('sid', 128)->primary();
+        $table->integer('internalKey')->default(0);
+        $table->string('username', 50)->default('');
+        $table->integer('lasthit')->default(0);
+        $table->string('action', 10)->default('');
+        $table->integer('id')->nullable();
+    });
+});
+
+afterEach(function () {
+    Capsule::connection()->disconnect();
+});
+
+function onSettings(int $userId, string $username, string $sid, int $lasthit, ?int $sessionHit): void
+{
+    ActiveUser::query()->insert(['sid' => $sid, 'internalKey' => $userId, 'username' => $username, 'lasthit' => $lasthit, 'action' => '17', 'id' => null]);
+    if ($sessionHit !== null) {
+        ActiveUserSession::query()->insert(['sid' => $sid, 'internalKey' => $userId, 'lasthit' => $sessionHit, 'ip' => '127.0.0.1']);
+    }
+}
+
+test('a manager who left the settings open no longer holds them once the session is gone', function () {
+    $now = 1_800_000_000;
+    $since = ActiveUserSession::validSince($now, 20);
+    // left 2 h ago, session already cleaned up
+    onSettings(2, 'gone', 'sid-gone', $now - 7200, null);
+    // last hit long ago, session row not cleaned yet but past the timeout
+    onSettings(3, 'expired', 'sid-expired', $now - 7200, $now - 21 * 60);
+
+    expect(ActiveUser::locked(17, null, 1, $since)->first())->toBeNull();
+});
+
+test('a signed-in manager on the settings page still holds them', function () {
+    $now = 1_800_000_000;
+    // opened the settings 30 min ago and kept the manager open: keep-alive refreshed the session
+    onSettings(2, 'editor', 'sid-editor', $now - 1800, $now - 60);
+
+    expect(ActiveUser::locked(17, null, 1, ActiveUserSession::validSince($now, 20))->first()?->username)->toBe('editor')
+        ->and(ActiveUser::locked(17, null, 2, ActiveUserSession::validSince($now, 20))->first())->toBeNull();
+});
+
+test('the session window is session_timeout minutes and never below two', function () {
+    expect(ActiveUserSession::validSince(10_000, 20))->toBe(10_000 - 1200)
+        ->and(ActiveUserSession::validSince(10_000, 0))->toBe(10_000 - 120)
+        ->and(ActiveUserSession::validSince(10_000, 1))->toBe(10_000 - 120);
+});
+
+test('the alert page waits for load, so a manager frame replaces alert() before it fires', function () {
+    $source = file_get_contents(dirname(__DIR__, 3) . '/src/Core.php');
+
+    expect($source)->toContain("window.addEventListener('load', function () { window.setTimeout(__alertQuit, 100); });")
+        ->and($source)->not->toContain("window.setTimeout('__alertQuit();',100);");
+});
+
+test('the dashboard does not send a new install to settings another manager holds', function () {
+    $view = file_get_contents(dirname(__DIR__, 4) . '/manager/views/page/2.blade.php');
+
+    expect($view)->toMatch('/settings_version.*\n.*!\\\\EvolutionCMS\\\\Models\\\\ActiveUser::locked\(17\)->exists\(\)/');
+});

manager/media/style/default/js/evo.js

@@ -2185,15 +2185,23 @@
                             history.go(1);
                         };
                         evo.alert(message);
-                        evo.getLockedElements(evo.getActionFromUrl(this.url), evo.main.getQueryVariable('id', this.url), function (data) {
-                            if (!!data || ~s.closeactions.indexOf(evo.getActionFromUrl(s.url))) {
-                                s.page.close();
-                                evo.tree.restoreTree();
-                            } else {
-                                w.main.location.href = evo.EVO_MANAGER_URL + s.url;
-                                w.history.replaceState(null, d.title, evo.tabsHistoryUrl(s.url));
-                            }
-                        });
+                        var alertAction = evo.getActionFromUrl(this.url),
+                            alertId = evo.main.getQueryVariable('id', this.url);
+                        // only an element page has a lock to look up; any other page that alerts (System Settings
+                        // held by another manager, no privileges) has nothing left to show, so its tab goes
+                        if (!evo.typesactions[alertAction] || !alertId) {
+                            s.page.close();
+                        } else {
+                            evo.getLockedElements(alertAction, alertId, function (data) {
+                                if (!!data || ~s.closeactions.indexOf(evo.getActionFromUrl(s.url))) {
+                                    s.page.close();
+                                    evo.tree.restoreTree();
+                                } else {
+                                    w.main.location.href = evo.EVO_MANAGER_URL + s.url;
+                                    w.history.replaceState(null, d.title, evo.tabsHistoryUrl(s.url));
+                                }
+                            });
+                        }
                     } else {
                         if (evo.getActionFromUrl(this.url, 2) || (~this.saveAndCloseActions.indexOf(evo.getActionFromUrl(this.url)) && parseInt(evo.main.getQueryVariable('r', this.url)))) {
                             this.close(e);

manager/views/page/2.blade.php

@@ -21,7 +21,9 @@
     <?php /*include_once evolutionCMS()->get('ManagerTheme')->getFileProcessor("actions/welcome.static.php");*/
     unset($_SESSION['itemname'], $_SESSION['itemaction']); // clear this, because it's only set for logging purposes
 
-    if (evo()->hasPermission('settings') && evo()->getConfig('settings_version') !== evo()->getVersionData('version')) {
+    // not while another manager holds the settings: their page would alert and go back here, and loop
+    if (evo()->hasPermission('settings') && evo()->getConfig('settings_version') !== evo()->getVersionData('version')
+        && !\EvolutionCMS\Models\ActiveUser::locked(17)->exists()) {
         // seems to be a new install - send the user to the configuration page
         exit('<script type="text/javascript">document.location.href="index.php?a=17";</script>');
     }