diff --git a/core/src/Core.php b/core/src/Core.php index f32768a5ca..8fd6669bce 100644 --- a/core/src/Core.php +++ b/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); }); @@ -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 diff --git a/core/src/Models/ActiveUser.php b/core/src/Models/ActiveUser.php index b03560f94d..757dddeb09 100644 --- a/core/src/Models/ActiveUser.php +++ b/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) { diff --git a/core/src/Models/ActiveUserSession.php b/core/src/Models/ActiveUserSession.php index e34c208aef..26c0d3d833 100644 --- a/core/src/Models/ActiveUserSession.php +++ b/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; + } } diff --git a/core/tests/Unit/Manager/SettingsLockTest.php b/core/tests/Unit/Manager/SettingsLockTest.php new file mode 100644 index 0000000000..88b93e8c61 --- /dev/null +++ b/core/tests/Unit/Manager/SettingsLockTest.php @@ -0,0 +1,82 @@ +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\(\)/'); +}); diff --git a/manager/media/style/default/js/evo.js b/manager/media/style/default/js/evo.js index 01d9e29539..a8d48c1921 100644 --- a/manager/media/style/default/js/evo.js +++ b/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); diff --git a/manager/views/page/2.blade.php b/manager/views/page/2.blade.php index 03dbef6c9c..e572ce5cac 100644 --- a/manager/views/page/2.blade.php +++ b/manager/views/page/2.blade.php @@ -21,7 +21,9 @@ 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(''); }