fix: use wasChanged() instead of isDirty() in updated hooks

Critical Bug Fix:
- isDirty() always returns false in updated() hook
- wasChanged() correctly tracks modifications after save

Files Fixed:
- ServerSetting: Sentinel restart now triggers on config changes
- DeletesUserSessions: Session invalidation now works on password change

Security Impact:
- CRITICAL: Password changes now properly invalidate user sessions
- Prevents session hijacking after password reset

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Andras Bacsai 2025-10-15 22:33:04 +02:00
parent 23c1184e86
commit e04b9cd07c
6 changed files with 363 additions and 7 deletions

View file

@ -35,7 +35,7 @@ class InstanceSettings extends Model
protected static function booted(): void
{
static::updated(function ($settings) {
if ($settings->isDirty('helper_version')) {
if ($settings->wasChanged('helper_version')) {
Server::chunkById(100, function ($servers) {
foreach ($servers as $server) {
PullHelperImageJob::dispatch($server);

View file

@ -79,11 +79,11 @@ protected static function booted()
});
static::updated(function ($settings) {
if (
$settings->isDirty('sentinel_token') ||
$settings->isDirty('sentinel_custom_url') ||
$settings->isDirty('sentinel_metrics_refresh_rate_seconds') ||
$settings->isDirty('sentinel_metrics_history_days') ||
$settings->isDirty('sentinel_push_interval_seconds')
$settings->wasChanged('sentinel_token') ||
$settings->wasChanged('sentinel_custom_url') ||
$settings->wasChanged('sentinel_metrics_refresh_rate_seconds') ||
$settings->wasChanged('sentinel_metrics_history_days') ||
$settings->wasChanged('sentinel_push_interval_seconds')
) {
$settings->server->restartSentinel();
}

View file

@ -26,7 +26,7 @@ protected static function bootDeletesUserSessions()
{
static::updated(function ($user) {
// Check if password was changed
if ($user->isDirty('password')) {
if ($user->wasChanged('password')) {
$user->deleteAllSessions();
}
});

View file

@ -0,0 +1,136 @@
<?php
use App\Models\User;
use Illuminate\Foundation\Testing\RefreshDatabase;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Hash;
uses(RefreshDatabase::class);
it('invalidates sessions when password changes', function () {
// Create a user
$user = User::factory()->create([
'password' => Hash::make('old-password'),
]);
// Create fake session records for the user
DB::table('sessions')->insert([
[
'id' => 'session-1',
'user_id' => $user->id,
'ip_address' => '127.0.0.1',
'user_agent' => 'Test Browser',
'payload' => base64_encode('test-payload-1'),
'last_activity' => now()->timestamp,
],
[
'id' => 'session-2',
'user_id' => $user->id,
'ip_address' => '127.0.0.1',
'user_agent' => 'Test Browser',
'payload' => base64_encode('test-payload-2'),
'last_activity' => now()->timestamp,
],
]);
// Verify sessions exist
expect(DB::table('sessions')->where('user_id', $user->id)->count())->toBe(2);
// Change password
$user->password = Hash::make('new-password');
$user->save();
// Verify all sessions for this user were deleted
expect(DB::table('sessions')->where('user_id', $user->id)->count())->toBe(0);
});
it('does not invalidate sessions when password is unchanged', function () {
// Create a user
$user = User::factory()->create([
'password' => Hash::make('password'),
]);
// Create fake session records for the user
DB::table('sessions')->insert([
[
'id' => 'session-1',
'user_id' => $user->id,
'ip_address' => '127.0.0.1',
'user_agent' => 'Test Browser',
'payload' => base64_encode('test-payload'),
'last_activity' => now()->timestamp,
],
]);
// Update other user fields (not password)
$user->name = 'New Name';
$user->save();
// Verify session still exists
expect(DB::table('sessions')->where('user_id', $user->id)->count())->toBe(1);
});
it('does not invalidate sessions when password is set to same value', function () {
// Create a user with a specific password
$hashedPassword = Hash::make('password');
$user = User::factory()->create([
'password' => $hashedPassword,
]);
// Create fake session records for the user
DB::table('sessions')->insert([
[
'id' => 'session-1',
'user_id' => $user->id,
'ip_address' => '127.0.0.1',
'user_agent' => 'Test Browser',
'payload' => base64_encode('test-payload'),
'last_activity' => now()->timestamp,
],
]);
// Set password to the same value
$user->password = $hashedPassword;
$user->save();
// Verify session still exists (password didn't actually change)
expect(DB::table('sessions')->where('user_id', $user->id)->count())->toBe(1);
});
it('invalidates sessions only for the user whose password changed', function () {
// Create two users
$user1 = User::factory()->create([
'password' => Hash::make('password1'),
]);
$user2 = User::factory()->create([
'password' => Hash::make('password2'),
]);
// Create sessions for both users
DB::table('sessions')->insert([
[
'id' => 'session-user1',
'user_id' => $user1->id,
'ip_address' => '127.0.0.1',
'user_agent' => 'Test Browser',
'payload' => base64_encode('test-payload-1'),
'last_activity' => now()->timestamp,
],
[
'id' => 'session-user2',
'user_id' => $user2->id,
'ip_address' => '127.0.0.1',
'user_agent' => 'Test Browser',
'payload' => base64_encode('test-payload-2'),
'last_activity' => now()->timestamp,
],
]);
// Change password for user1 only
$user1->password = Hash::make('new-password1');
$user1->save();
// Verify user1's sessions were deleted but user2's remain
expect(DB::table('sessions')->where('user_id', $user1->id)->count())->toBe(0);
expect(DB::table('sessions')->where('user_id', $user2->id)->count())->toBe(1);
});

View file

@ -0,0 +1,81 @@
<?php
use App\Jobs\PullHelperImageJob;
use App\Models\InstanceSettings;
use App\Models\Server;
use App\Models\User;
use Illuminate\Foundation\Testing\RefreshDatabase;
use Illuminate\Support\Facades\Queue;
uses(RefreshDatabase::class);
it('dispatches PullHelperImageJob when helper_version changes', function () {
Queue::fake();
// Create user and servers
$user = User::factory()->create();
$team = $user->teams()->first();
Server::factory()->count(3)->create(['team_id' => $team->id]);
$settings = InstanceSettings::firstOrCreate([], ['helper_version' => 'v1.0.0']);
// Change helper_version
$settings->helper_version = 'v1.2.3';
$settings->save();
// Verify PullHelperImageJob was dispatched for all servers
Queue::assertPushed(PullHelperImageJob::class, 3);
});
it('does not dispatch PullHelperImageJob when helper_version is unchanged', function () {
Queue::fake();
// Create user and servers
$user = User::factory()->create();
$team = $user->teams()->first();
Server::factory()->count(3)->create(['team_id' => $team->id]);
$settings = InstanceSettings::firstOrCreate([], ['helper_version' => 'v1.0.0']);
$currentVersion = $settings->helper_version;
// Set to same value
$settings->helper_version = $currentVersion;
$settings->save();
// Verify no jobs were dispatched
Queue::assertNotPushed(PullHelperImageJob::class);
});
it('does not dispatch PullHelperImageJob when other fields change', function () {
Queue::fake();
// Create user and servers
$user = User::factory()->create();
$team = $user->teams()->first();
Server::factory()->count(3)->create(['team_id' => $team->id]);
$settings = InstanceSettings::firstOrCreate([], ['helper_version' => 'v1.0.0']);
// Change different field
$settings->is_auto_update_enabled = ! $settings->is_auto_update_enabled;
$settings->save();
// Verify no jobs were dispatched
Queue::assertNotPushed(PullHelperImageJob::class);
});
it('detects helper_version changes with wasChanged', function () {
$changeDetected = false;
InstanceSettings::updated(function ($settings) use (&$changeDetected) {
if ($settings->wasChanged('helper_version')) {
$changeDetected = true;
}
});
$settings = InstanceSettings::firstOrCreate([], ['helper_version' => 'v1.0.0']);
$settings->helper_version = 'v2.0.0';
$settings->save();
expect($changeDetected)->toBeTrue();
});

View file

@ -0,0 +1,139 @@
<?php
use App\Models\Server;
use App\Models\ServerSetting;
use App\Models\User;
use Illuminate\Foundation\Testing\RefreshDatabase;
uses(RefreshDatabase::class);
beforeEach(function () {
// Create user (which automatically creates a team)
$user = User::factory()->create();
$this->team = $user->teams()->first();
// Create server with the team
$this->server = Server::factory()->create([
'team_id' => $this->team->id,
]);
});
it('detects sentinel_token changes with wasChanged', function () {
$changeDetected = false;
// Register a test listener that will be called after the model's booted listeners
ServerSetting::updated(function ($settings) use (&$changeDetected) {
if ($settings->wasChanged('sentinel_token')) {
$changeDetected = true;
}
});
$settings = $this->server->settings;
$settings->sentinel_token = 'new-token-value';
$settings->save();
expect($changeDetected)->toBeTrue();
});
it('detects sentinel_custom_url changes with wasChanged', function () {
$changeDetected = false;
ServerSetting::updated(function ($settings) use (&$changeDetected) {
if ($settings->wasChanged('sentinel_custom_url')) {
$changeDetected = true;
}
});
$settings = $this->server->settings;
$settings->sentinel_custom_url = 'https://new-url.com';
$settings->save();
expect($changeDetected)->toBeTrue();
});
it('detects sentinel_metrics_refresh_rate_seconds changes with wasChanged', function () {
$changeDetected = false;
ServerSetting::updated(function ($settings) use (&$changeDetected) {
if ($settings->wasChanged('sentinel_metrics_refresh_rate_seconds')) {
$changeDetected = true;
}
});
$settings = $this->server->settings;
$settings->sentinel_metrics_refresh_rate_seconds = 60;
$settings->save();
expect($changeDetected)->toBeTrue();
});
it('detects sentinel_metrics_history_days changes with wasChanged', function () {
$changeDetected = false;
ServerSetting::updated(function ($settings) use (&$changeDetected) {
if ($settings->wasChanged('sentinel_metrics_history_days')) {
$changeDetected = true;
}
});
$settings = $this->server->settings;
$settings->sentinel_metrics_history_days = 14;
$settings->save();
expect($changeDetected)->toBeTrue();
});
it('detects sentinel_push_interval_seconds changes with wasChanged', function () {
$changeDetected = false;
ServerSetting::updated(function ($settings) use (&$changeDetected) {
if ($settings->wasChanged('sentinel_push_interval_seconds')) {
$changeDetected = true;
}
});
$settings = $this->server->settings;
$settings->sentinel_push_interval_seconds = 30;
$settings->save();
expect($changeDetected)->toBeTrue();
});
it('does not detect changes when unrelated field is changed', function () {
$changeDetected = false;
ServerSetting::updated(function ($settings) use (&$changeDetected) {
if (
$settings->wasChanged('sentinel_token') ||
$settings->wasChanged('sentinel_custom_url') ||
$settings->wasChanged('sentinel_metrics_refresh_rate_seconds') ||
$settings->wasChanged('sentinel_metrics_history_days') ||
$settings->wasChanged('sentinel_push_interval_seconds')
) {
$changeDetected = true;
}
});
$settings = $this->server->settings;
$settings->is_reachable = ! $settings->is_reachable;
$settings->save();
expect($changeDetected)->toBeFalse();
});
it('does not detect changes when sentinel field is set to same value', function () {
$changeDetected = false;
ServerSetting::updated(function ($settings) use (&$changeDetected) {
if ($settings->wasChanged('sentinel_token')) {
$changeDetected = true;
}
});
$settings = $this->server->settings;
$currentToken = $settings->sentinel_token;
$settings->sentinel_token = $currentToken;
$settings->save();
expect($changeDetected)->toBeFalse();
});