From c0a81a0c5077b7fe1eeb1e04ead33c188a12db46 Mon Sep 17 00:00:00 2001 From: sinach Date: Sat, 15 Mar 2025 10:29:43 +0100 Subject: [PATCH 1/8] feat: added login tracking and login count features to User model --- .lando.dist.yml => .lando.yml | 0 ..._15_000000_add_login_tracking_to_users.php | 22 +++++++ src/EclipseServiceProvider.php | 3 + src/Filament/Resources/UserResource.php | 14 +++++ src/Models/User.php | 17 +++++ tests/Feature/LoginTrackingTest.php | 62 +++++++++++++++++++ 6 files changed, 118 insertions(+) rename .lando.dist.yml => .lando.yml (100%) create mode 100644 database/migrations/2025_03_15_000000_add_login_tracking_to_users.php create mode 100644 tests/Feature/LoginTrackingTest.php diff --git a/.lando.dist.yml b/.lando.yml similarity index 100% rename from .lando.dist.yml rename to .lando.yml diff --git a/database/migrations/2025_03_15_000000_add_login_tracking_to_users.php b/database/migrations/2025_03_15_000000_add_login_tracking_to_users.php new file mode 100644 index 0000000..103c22e --- /dev/null +++ b/database/migrations/2025_03_15_000000_add_login_tracking_to_users.php @@ -0,0 +1,22 @@ +dateTime('last_login_at')->nullable(); + $table->integer('login_count')->default(0); + }); + } + + public function down() + { + Schema::table('users', function (Blueprint $table) { + $table->dropColumn(['last_login_at', 'login_count']); + }); + } +}; diff --git a/src/EclipseServiceProvider.php b/src/EclipseServiceProvider.php index 7d8089e..91fe09a 100644 --- a/src/EclipseServiceProvider.php +++ b/src/EclipseServiceProvider.php @@ -27,6 +27,9 @@ public function configurePackage(Package $package): void 'permission', 'telescope', ]) + ->hasMigrations([ + 'add_login_tracking_to_users', + ]) ->discoversMigrations() ->runsMigrations() ->hasTranslations(); diff --git a/src/Filament/Resources/UserResource.php b/src/Filament/Resources/UserResource.php index 470a480..0461a24 100644 --- a/src/Filament/Resources/UserResource.php +++ b/src/Filament/Resources/UserResource.php @@ -86,6 +86,16 @@ public static function table(Table $table): Table ->searchable() ->sortable() ->toggleable(), + Tables\Columns\TextColumn::make('last_login_at') + ->label('Last login') + ->dateTime() + ->sortable() + ->toggleable(), + Tables\Columns\TextColumn::make('login_count') + ->label('Total Logins') + ->sortable() + ->numeric() + ->formatStateUsing(fn (?int $state) => $state ?? 0), ]; if (config('eclipse.email_verification')) { @@ -145,6 +155,10 @@ public static function table(Table $table): Table ->label('Last name'), TextConstraint::make('name') ->label('Full name'), + TextConstraint::make('last_login_at') + ->label('Last login Date'), + TextConstraint::make('login_count') + ->label('Total Logins'), ]), ]; diff --git a/src/Models/User.php b/src/Models/User.php index 252ba57..046db18 100644 --- a/src/Models/User.php +++ b/src/Models/User.php @@ -44,6 +44,8 @@ class User extends Authenticatable implements FilamentUser, HasAvatar, HasMedia, 'last_name', 'email', 'password', + 'last_login_at', + 'login_count', ]; /** @@ -66,6 +68,7 @@ protected function casts(): array return [ 'email_verified_at' => 'datetime', 'password' => 'hashed', + 'last_login_at' => 'datetime', ]; } @@ -109,5 +112,19 @@ protected static function booted() static::saving(function (self $user) { $user->name = trim("$user->first_name $user->last_name"); }); + + static::creating(function (self $user) { + if (is_null($user->login_count)) { + $user->login_count = 0; + } + }); + } + + // Add the following method to the User model: last_login_at & login_count features + public function updateLoginTracking() + { + $this->last_login_at = now(); + $this->increment('login_count'); + $this->save(); } } diff --git a/tests/Feature/LoginTrackingTest.php b/tests/Feature/LoginTrackingTest.php new file mode 100644 index 0000000..60d916b --- /dev/null +++ b/tests/Feature/LoginTrackingTest.php @@ -0,0 +1,62 @@ +create(); + + expect($user->last_login_at)->toBeNull(); + expect($user->login_count ?? 0)->toBe(0); +}); + +test('user login updates last login timestamp and increments count', function () { + $user = User::factory()->create([ + 'last_login_at' => null, + 'login_count' => 0, + ]); + + // Simulate login + Auth::login($user); + $user->updateLoginTracking(); + $user->refresh(); + + expect($user->last_login_at)->not->toBeNull(); + expect($user->login_count)->toBe(1); +}); + +test('multiple logins correctly increment login count', function () { + $user = User::factory()->create([ + 'login_count' => 2, // User has logged in twice before + ]); + + Auth::login($user); + $user->updateLoginTracking(); + $user->refresh(); + + expect($user->login_count)->toBe(3); // Should increase by 1 +}); + +test('login tracking does not reset on logout', function () { + $user = User::factory()->create([ + 'last_login_at' => now()->subDays(1), + 'login_count' => 5, + ]); + + Auth::login($user); + $user->updateLoginTracking(); + Auth::logout(); + $user->refresh(); + + expect($user->last_login_at)->not->toBeNull(); + expect($user->login_count)->toBe(6); // Login count should remain after logout +}); + +test('guest users do not have login tracking data', function () { + $this->get('/admin')->assertRedirect('admin/login'); + + expect(Auth::user())->toBeNull(); +}); From abe1425d6c832761b7e2b6a24c60c218447a3973 Mon Sep 17 00:00:00 2001 From: sinach Date: Sat, 15 Mar 2025 11:15:36 +0100 Subject: [PATCH 2/8] feat: added user trash & restore feature --- ...03_15_000001_add_soft_deletes_to_users.php | 21 ++++++++ src/Filament/Resources/UserResource.php | 14 +++++- src/Models/User.php | 20 +++++++- .../Filament/Resources/UserResourceTest.php | 6 ++- tests/Feature/UserTrashRestoreTest.php | 49 +++++++++++++++++++ 5 files changed, 107 insertions(+), 3 deletions(-) create mode 100644 database/migrations/2025_03_15_000001_add_soft_deletes_to_users.php create mode 100644 tests/Feature/UserTrashRestoreTest.php diff --git a/database/migrations/2025_03_15_000001_add_soft_deletes_to_users.php b/database/migrations/2025_03_15_000001_add_soft_deletes_to_users.php new file mode 100644 index 0000000..10ba119 --- /dev/null +++ b/database/migrations/2025_03_15_000001_add_soft_deletes_to_users.php @@ -0,0 +1,21 @@ +softDeletes(); // ✅ Adds `deleted_at` column + }); + } + + public function down() + { + Schema::table('users', function (Blueprint $table) { + $table->dropSoftDeletes(); + }); + } +}; diff --git a/src/Filament/Resources/UserResource.php b/src/Filament/Resources/UserResource.php index 0461a24..82b4630 100644 --- a/src/Filament/Resources/UserResource.php +++ b/src/Filament/Resources/UserResource.php @@ -32,6 +32,8 @@ class UserResource extends Resource implements HasShieldPermissions protected static ?string $recordTitleAttribute = 'first_name'; + protected static bool $softDeletes = true; + public static function form(Form $form): Form { return $form->schema([ @@ -160,6 +162,9 @@ public static function table(Table $table): Table TextConstraint::make('login_count') ->label('Total Logins'), ]), + + // added trash filter + Tables\Filters\TrashedFilter::make() ]; return $table @@ -169,7 +174,14 @@ public static function table(Table $table): Table Tables\Actions\ActionGroup::make([ Tables\Actions\ViewAction::make(), Tables\Actions\EditAction::make(), - Tables\Actions\DeleteAction::make()->disabled(fn (User $user) => $user->id === auth()->user()->id), + Tables\Actions\DeleteAction::make()->disabled(fn (User $user) => $user->id === auth()->user()->id) + ->visible(fn (User $user) => $user->id !== auth()->user()->id) + ->authorize(fn () => auth()->user()->can('delete', User::class)) + ->requiresConfirmation(), + Tables\Actions\RestoreAction::make() + ->visible(fn (User $user) => $user->trashed()) + ->authorize(fn () => auth()->user()->can('restore', User::class)) + ->requiresConfirmation(), ]), ]) ->bulkActions([ diff --git a/src/Models/User.php b/src/Models/User.php index 046db18..5bca82e 100644 --- a/src/Models/User.php +++ b/src/Models/User.php @@ -15,6 +15,7 @@ use Spatie\MediaLibrary\HasMedia; use Spatie\MediaLibrary\InteractsWithMedia; use Spatie\Permission\Traits\HasRoles; +use Illuminate\Database\Eloquent\SoftDeletes; /** * @property int $id @@ -30,9 +31,10 @@ */ class User extends Authenticatable implements FilamentUser, HasAvatar, HasMedia, HasTenants { - use HasFactory, HasRoles, InteractsWithMedia, Notifiable; + use HasFactory, HasRoles, InteractsWithMedia, Notifiable, SoftDeletes; protected $table = 'users'; + protected $dates = ['deleted_at']; /** * The attributes that are mass assignable. @@ -118,6 +120,12 @@ protected static function booted() $user->login_count = 0; } }); + + static::retrieved(function (self $user) { + if ($user->trashed() && request()->routeIs('login')) { + throw new \Exception('This account has been deactivated.'); + } + }); } // Add the following method to the User model: last_login_at & login_count features @@ -127,4 +135,14 @@ public function updateLoginTracking() $this->increment('login_count'); $this->save(); } + + // Add the following method to the User model: delete method + public function delete() + { + if ($this->id === auth()->id()) { + throw new \Exception('You cannot delete your own account.'); + } + + return parent::delete(); + } } diff --git a/tests/Feature/Filament/Resources/UserResourceTest.php b/tests/Feature/Filament/Resources/UserResourceTest.php index b5f58d9..1c95536 100644 --- a/tests/Feature/Filament/Resources/UserResourceTest.php +++ b/tests/Feature/Filament/Resources/UserResourceTest.php @@ -130,7 +130,11 @@ ->assertTableActionEnabled(DeleteAction::class, $user) ->callTableAction(DeleteAction::class, $user); - $this->assertModelMissing($user); + // $this->assertModelMissing($user); + + // replaced the above line with the following because this properly checks that the user is soft deleted instead of completely removed from the database. + + $this->assertSoftDeleted('users', ['id' => $user->id]); }); test('authed user cannot delete himself', function () { diff --git a/tests/Feature/UserTrashRestoreTest.php b/tests/Feature/UserTrashRestoreTest.php new file mode 100644 index 0000000..4c1f72b --- /dev/null +++ b/tests/Feature/UserTrashRestoreTest.php @@ -0,0 +1,49 @@ +create(); + $user = User::factory()->create(); + + // Simulate admin permission + Auth::login($admin); + + $user->delete(); + + expect($user->fresh()->trashed())->toBeTrue(); +}); + +test('user cannot trash himself', function () { + $user = User::factory()->create(); + + Auth::login($user); + + $this->expectException(\Exception::class); + $user->delete(); +}); + +test('authorized user can restore a trashed user', function () { + $admin = User::factory()->create(); + $user = User::factory()->create(); + $user->delete(); // Move to trash + + Auth::login($admin); + + $user->restore(); // Restore user + + expect($user->fresh()->trashed())->toBeFalse(); +}); + +test('trashed user cannot login', function () { + $user = User::factory()->create(); + $user->delete(); // Move to trash + + $attempt = Auth::attempt(['email' => $user->email, 'password' => 'password']); + + expect($attempt)->toBeFalse(); +}); From 1bb7d264b0a44556ba12fcb85281d656764ff757 Mon Sep 17 00:00:00 2001 From: sinach Date: Sat, 15 Mar 2025 15:48:52 +0100 Subject: [PATCH 3/8] fix: made code fixes to the corrections provided for login tracking --- .lando.yml => .lando.dist.yml | 0 database/factories/UserFactory.php | 1 + src/EclipseServiceProvider.php | 12 ++++++++--- src/Models/User.php | 19 +++++++++-------- tests/Feature/LoginTrackingTest.php | 32 ++--------------------------- 5 files changed, 23 insertions(+), 41 deletions(-) rename .lando.yml => .lando.dist.yml (100%) diff --git a/.lando.yml b/.lando.dist.yml similarity index 100% rename from .lando.yml rename to .lando.dist.yml diff --git a/database/factories/UserFactory.php b/database/factories/UserFactory.php index c1d68be..9cf74b3 100644 --- a/database/factories/UserFactory.php +++ b/database/factories/UserFactory.php @@ -38,6 +38,7 @@ public function definition(): array 'email_verified_at' => now(), 'password' => static::$password ??= Hash::make('password'), 'remember_token' => Str::random(10), + 'login_count' => 0, ]; } diff --git a/src/EclipseServiceProvider.php b/src/EclipseServiceProvider.php index 91fe09a..51f0f72 100644 --- a/src/EclipseServiceProvider.php +++ b/src/EclipseServiceProvider.php @@ -5,8 +5,11 @@ use Eclipse\Core\Console\Commands\ClearCommand; use Eclipse\Core\Console\Commands\DeployCommand; use Eclipse\Core\Console\Commands\PostComposerUpdate; +use Eclipse\Core\Models\User; use Eclipse\Core\Providers\AdminPanelProvider; use Eclipse\Core\Providers\TelescopeServiceProvider; +use Illuminate\Auth\Events\Login; +use Illuminate\Support\Facades\Event; use Spatie\LaravelPackageTools\Package; use Spatie\LaravelPackageTools\PackageServiceProvider; @@ -27,9 +30,6 @@ public function configurePackage(Package $package): void 'permission', 'telescope', ]) - ->hasMigrations([ - 'add_login_tracking_to_users', - ]) ->discoversMigrations() ->runsMigrations() ->hasTranslations(); @@ -41,6 +41,12 @@ public function register() require_once __DIR__.'/Helpers/helpers.php'; + Event::listen(Login::class, function ($event) { + if ($event->user instanceof User) { + $event->user->updateLoginTracking(); + } + }); + $this->app->register(AdminPanelProvider::class); if ($this->app->environment('local') && class_exists(\Laravel\Telescope\TelescopeServiceProvider::class)) { diff --git a/src/Models/User.php b/src/Models/User.php index 5bca82e..0af1eda 100644 --- a/src/Models/User.php +++ b/src/Models/User.php @@ -115,12 +115,6 @@ protected static function booted() $user->name = trim("$user->first_name $user->last_name"); }); - static::creating(function (self $user) { - if (is_null($user->login_count)) { - $user->login_count = 0; - } - }); - static::retrieved(function (self $user) { if ($user->trashed() && request()->routeIs('login')) { throw new \Exception('This account has been deactivated.'); @@ -128,7 +122,11 @@ protected static function booted() }); } - // Add the following method to the User model: last_login_at & login_count features + /** + * Update the user's last login timestamp and increment login count. + * + * @return void + */ public function updateLoginTracking() { $this->last_login_at = now(); @@ -136,7 +134,12 @@ public function updateLoginTracking() $this->save(); } - // Add the following method to the User model: delete method + /** + * Delete the user account, preventing self-deletion. + * + * @throws \Exception If the user attempts to delete their own account. + * @return bool|null + */ public function delete() { if ($this->id === auth()->id()) { diff --git a/tests/Feature/LoginTrackingTest.php b/tests/Feature/LoginTrackingTest.php index 60d916b..54cdab4 100644 --- a/tests/Feature/LoginTrackingTest.php +++ b/tests/Feature/LoginTrackingTest.php @@ -10,7 +10,7 @@ $user = User::factory()->create(); expect($user->last_login_at)->toBeNull(); - expect($user->login_count ?? 0)->toBe(0); + expect($user->login_count)->toBe(0); }); test('user login updates last login timestamp and increments count', function () { @@ -21,40 +21,12 @@ // Simulate login Auth::login($user); - $user->updateLoginTracking(); - $user->refresh(); + $user->refresh(); // Reload from DB to reflect changes expect($user->last_login_at)->not->toBeNull(); expect($user->login_count)->toBe(1); }); -test('multiple logins correctly increment login count', function () { - $user = User::factory()->create([ - 'login_count' => 2, // User has logged in twice before - ]); - - Auth::login($user); - $user->updateLoginTracking(); - $user->refresh(); - - expect($user->login_count)->toBe(3); // Should increase by 1 -}); - -test('login tracking does not reset on logout', function () { - $user = User::factory()->create([ - 'last_login_at' => now()->subDays(1), - 'login_count' => 5, - ]); - - Auth::login($user); - $user->updateLoginTracking(); - Auth::logout(); - $user->refresh(); - - expect($user->last_login_at)->not->toBeNull(); - expect($user->login_count)->toBe(6); // Login count should remain after logout -}); - test('guest users do not have login tracking data', function () { $this->get('/admin')->assertRedirect('admin/login'); From 201414184b0c354dffc4b61941373b25b6b9b3a6 Mon Sep 17 00:00:00 2001 From: sinach Date: Sat, 15 Mar 2025 23:05:27 +0100 Subject: [PATCH 4/8] refactor: modified testcases to explicitly check for permission & added new testcases as well --- src/Filament/Resources/UserResource.php | 11 +-- src/Models/User.php | 27 ++++++- .../Filament/Resources/UserResourceTest.php | 14 +++- tests/Feature/UserTrashRestoreTest.php | 74 +++++++++++++++---- 4 files changed, 100 insertions(+), 26 deletions(-) diff --git a/src/Filament/Resources/UserResource.php b/src/Filament/Resources/UserResource.php index 82b4630..79796df 100644 --- a/src/Filament/Resources/UserResource.php +++ b/src/Filament/Resources/UserResource.php @@ -32,7 +32,7 @@ class UserResource extends Resource implements HasShieldPermissions protected static ?string $recordTitleAttribute = 'first_name'; - protected static bool $softDeletes = true; + protected static bool $softDelete = true; public static function form(Form $form): Form { @@ -174,13 +174,13 @@ public static function table(Table $table): Table Tables\Actions\ActionGroup::make([ Tables\Actions\ViewAction::make(), Tables\Actions\EditAction::make(), - Tables\Actions\DeleteAction::make()->disabled(fn (User $user) => $user->id === auth()->user()->id) - ->visible(fn (User $user) => $user->id !== auth()->user()->id) - ->authorize(fn () => auth()->user()->can('delete', User::class)) + Tables\Actions\DeleteAction::make() + ->authorize(fn () => auth()->user()?->hasPermissionTo('delete users')) + ->visible(fn () => auth()->user()?->hasPermissionTo('delete users')) ->requiresConfirmation(), Tables\Actions\RestoreAction::make() ->visible(fn (User $user) => $user->trashed()) - ->authorize(fn () => auth()->user()->can('restore', User::class)) + ->authorize(fn () => auth()->user()->can('restore users', User::class)) ->requiresConfirmation(), ]), ]) @@ -293,6 +293,7 @@ public static function getPermissionPrefixes(): array 'update', 'delete', 'delete_any', + 'restore', ]; } } diff --git a/src/Models/User.php b/src/Models/User.php index 0af1eda..9f52ef6 100644 --- a/src/Models/User.php +++ b/src/Models/User.php @@ -16,6 +16,7 @@ use Spatie\MediaLibrary\InteractsWithMedia; use Spatie\Permission\Traits\HasRoles; use Illuminate\Database\Eloquent\SoftDeletes; +use Spatie\Permission\Exceptions\UnauthorizedException; /** * @property int $id @@ -28,6 +29,7 @@ * @property string|null $remember_token * @property string|null $created_at * @property string|null $updated_at + * @property string|null $deleted_at */ class User extends Authenticatable implements FilamentUser, HasAvatar, HasMedia, HasTenants { @@ -116,10 +118,10 @@ protected static function booted() }); static::retrieved(function (self $user) { - if ($user->trashed() && request()->routeIs('login')) { + if ($user->trashed() && auth()->check() && request()->routeIs('login')) { throw new \Exception('This account has been deactivated.'); } - }); + }); } /** @@ -140,12 +142,29 @@ public function updateLoginTracking() * @throws \Exception If the user attempts to delete their own account. * @return bool|null */ - public function delete() + public function delete(): ?bool { - if ($this->id === auth()->id()) { + $authUser = auth()->user(); + + if (!$authUser || !$authUser->hasAnyRole(['super-admin']) && !$authUser->hasPermissionTo('delete users')) { + throw new UnauthorizedException(403, 'You do not have permission to delete users.'); + } + + if ($this->id === $authUser->id) { throw new \Exception('You cannot delete your own account.'); } return parent::delete(); } + + public function restore(): bool + { + $authUser = auth()->user(); + + if (!$authUser || !$authUser->hasAnyRole(['super-admin']) && !$authUser->hasPermissionTo('restore users')) { + throw new UnauthorizedException(403, 'You do not have permission to restore users.'); + } + + return parent::restore(); + } } diff --git a/tests/Feature/Filament/Resources/UserResourceTest.php b/tests/Feature/Filament/Resources/UserResourceTest.php index 1c95536..b45d261 100644 --- a/tests/Feature/Filament/Resources/UserResourceTest.php +++ b/tests/Feature/Filament/Resources/UserResourceTest.php @@ -7,11 +7,16 @@ use Filament\Tables\Actions\DeleteAction; use Filament\Tables\Actions\DeleteBulkAction; use Illuminate\Support\Facades\Hash; +use Spatie\Permission\Models\Permission; use function Pest\Livewire\livewire; beforeEach(function () { $this->set_up_super_admin_and_tenant(); + + collect(['delete users', 'restore users'])->each(fn ($permission) => + Permission::firstOrCreate(['name' => $permission, 'guard_name' => 'web']) + ); }); test('authorized access can be allowed', function () { @@ -123,9 +128,16 @@ }); test('user can be deleted', function () { - $user = User::factory()->create(); + $this->set_up_super_admin_and_tenant(); + $admin = auth()->user(); + + $admin->syncPermissions(['delete users']); + + $user = User::factory()->create(); + livewire(ListUsers::class) + ->assertSuccessful() ->assertTableActionExists(DeleteAction::class) ->assertTableActionEnabled(DeleteAction::class, $user) ->callTableAction(DeleteAction::class, $user); diff --git a/tests/Feature/UserTrashRestoreTest.php b/tests/Feature/UserTrashRestoreTest.php index 4c1f72b..e6de507 100644 --- a/tests/Feature/UserTrashRestoreTest.php +++ b/tests/Feature/UserTrashRestoreTest.php @@ -3,47 +3,89 @@ use Eclipse\Core\Models\User; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\Auth; +use Spatie\Permission\Exceptions\UnauthorizedException; +use Spatie\Permission\Models\Permission; uses(RefreshDatabase::class); -test('authorized user can trash another user', function () { - $admin = User::factory()->create(); +beforeEach(function () { + $this->set_up_super_admin_and_tenant(); + $this->admin = auth()->user(); + + Permission::firstOrCreate(['name' => 'delete users']); + Permission::firstOrCreate(['name' => 'restore users']); + + $this->admin->syncPermissions(['delete users', 'restore users']); +}); + +test('authorized user with permission can trash another user', function () { $user = User::factory()->create(); - // Simulate admin permission - Auth::login($admin); - + Auth::login($this->admin); + + expect($this->admin->can('delete users'))->toBeTrue(); + $user->delete(); expect($user->fresh()->trashed())->toBeTrue(); }); -test('user cannot trash himself', function () { +test('non-authorized user cannot trash another user', function () { $user = User::factory()->create(); + $otherUser = User::factory()->create(); Auth::login($user); + expect($user->can('delete users'))->toBeFalse(); + + $this->expectException(UnauthorizedException::class); + + $otherUser->delete(); +}); + +test('user cannot trash himself', function () { + Auth::login($this->admin); + $this->expectException(\Exception::class); - $user->delete(); + + $this->admin->delete(); }); -test('authorized user can restore a trashed user', function () { - $admin = User::factory()->create(); +test('authorized user with permission can restore a trashed user', function () { $user = User::factory()->create(); - $user->delete(); // Move to trash + $user->delete(); + + Auth::login($this->admin); - Auth::login($admin); + expect($this->admin->can('restore users'))->toBeTrue(); - $user->restore(); // Restore user + $user->restore(); expect($user->fresh()->trashed())->toBeFalse(); }); +test('non-authorized user cannot restore another user', function () { + $userToTrash = User::factory()->create(); + $userToTrash->delete(); + + $nonAuthorizedUser = User::factory()->create(); + + Auth::login($nonAuthorizedUser); + + expect($nonAuthorizedUser->can('restore users'))->toBeFalse(); + + $this->expectException(UnauthorizedException::class); + + $userToTrash->restore(); +}); + test('trashed user cannot login', function () { - $user = User::factory()->create(); - $user->delete(); // Move to trash + $userToTrash = User::factory()->create(); + $userToTrash->delete(); + + Auth::logout(); - $attempt = Auth::attempt(['email' => $user->email, 'password' => 'password']); + $attempt = Auth::attempt(['email' => $userToTrash->email, 'password' => 'password']); expect($attempt)->toBeFalse(); -}); +}); \ No newline at end of file From 3241d643d9095c7ab82a9615a87208a486920915 Mon Sep 17 00:00:00 2001 From: sinach Date: Tue, 18 Mar 2025 00:35:24 +0100 Subject: [PATCH 5/8] fix: rewrote the user trash restore tests to use filament policies --- src/Filament/Resources/UserResource.php | 13 +- src/Models/User.php | 14 +- src/Policies/UserPolicy.php | 42 +++- .../Filament/Resources/UserResourceTest.php | 10 +- tests/Feature/UserTrashRestoreTest.php | 205 +++++++++++++++--- 5 files changed, 226 insertions(+), 58 deletions(-) diff --git a/src/Filament/Resources/UserResource.php b/src/Filament/Resources/UserResource.php index 79796df..42fe38d 100644 --- a/src/Filament/Resources/UserResource.php +++ b/src/Filament/Resources/UserResource.php @@ -175,13 +175,11 @@ public static function table(Table $table): Table Tables\Actions\ViewAction::make(), Tables\Actions\EditAction::make(), Tables\Actions\DeleteAction::make() - ->authorize(fn () => auth()->user()?->hasPermissionTo('delete users')) - ->visible(fn () => auth()->user()?->hasPermissionTo('delete users')) - ->requiresConfirmation(), + ->authorize(fn (User $record) => auth()->user()->can('delete_user') && auth()->id() !== $record->id) + ->requiresConfirmation(), Tables\Actions\RestoreAction::make() - ->visible(fn (User $user) => $user->trashed()) - ->authorize(fn () => auth()->user()->can('restore users', User::class)) - ->requiresConfirmation(), + ->visible(fn (User $user) => $user->trashed() && auth()->user()->can('restore_user')) + ->requiresConfirmation(), ]), ]) ->bulkActions([ @@ -294,6 +292,9 @@ public static function getPermissionPrefixes(): array 'delete', 'delete_any', 'restore', + 'restore_any', + 'force_delete', + 'force_delete_any', ]; } } diff --git a/src/Models/User.php b/src/Models/User.php index 9f52ef6..e2f9743 100644 --- a/src/Models/User.php +++ b/src/Models/User.php @@ -144,13 +144,7 @@ public function updateLoginTracking() */ public function delete(): ?bool { - $authUser = auth()->user(); - - if (!$authUser || !$authUser->hasAnyRole(['super-admin']) && !$authUser->hasPermissionTo('delete users')) { - throw new UnauthorizedException(403, 'You do not have permission to delete users.'); - } - - if ($this->id === $authUser->id) { + if ($this->id === auth()->id()) { throw new \Exception('You cannot delete your own account.'); } @@ -159,12 +153,6 @@ public function delete(): ?bool public function restore(): bool { - $authUser = auth()->user(); - - if (!$authUser || !$authUser->hasAnyRole(['super-admin']) && !$authUser->hasPermissionTo('restore users')) { - throw new UnauthorizedException(403, 'You do not have permission to restore users.'); - } - return parent::restore(); } } diff --git a/src/Policies/UserPolicy.php b/src/Policies/UserPolicy.php index b07f6b3..415dbb0 100644 --- a/src/Policies/UserPolicy.php +++ b/src/Policies/UserPolicy.php @@ -44,9 +44,13 @@ public function update(User $user): bool /** * Determine whether the user can delete the model. */ - public function delete(User $user): bool + public function delete(User $authenticatedUser, User $user): bool { - return $user->can('delete_user'); + if ($authenticatedUser->id === $user->id) { + return false; + } + + return $authenticatedUser->can('delete_user'); } /** @@ -56,4 +60,36 @@ public function deleteAny(User $user): bool { return $user->can('delete_any_user'); } -} + + /** + * Determine whether the user can restore the model. + */ + public function restore(User $user): bool + { + return $user->can('restore_user'); + } + + /** + * Determine whether the user can bulk restore. + */ + public function restoreAny(User $user): bool + { + return $user->can('restore_any_user'); + } + + /** + * Determine whether the user can permanently delete the model. + */ + public function forceDelete(User $user): bool + { + return $user->can('force_delete_user'); + } + + /** + * Determine whether the user can permanently bulk delete. + */ + public function forceDeleteAny(User $user): bool + { + return $user->can('force_delete_any_user'); + } +} \ No newline at end of file diff --git a/tests/Feature/Filament/Resources/UserResourceTest.php b/tests/Feature/Filament/Resources/UserResourceTest.php index b45d261..cee5cc1 100644 --- a/tests/Feature/Filament/Resources/UserResourceTest.php +++ b/tests/Feature/Filament/Resources/UserResourceTest.php @@ -14,9 +14,11 @@ beforeEach(function () { $this->set_up_super_admin_and_tenant(); - collect(['delete users', 'restore users'])->each(fn ($permission) => + collect(['delete_user', 'restore_user'])->each(fn ($permission) => Permission::firstOrCreate(['name' => $permission, 'guard_name' => 'web']) ); + + auth()->user()->syncPermissions(['delete_user', 'restore_user']); }); test('authorized access can be allowed', function () { @@ -132,7 +134,7 @@ $admin = auth()->user(); - $admin->syncPermissions(['delete users']); + $admin->syncPermissions(['delete_user']); $user = User::factory()->create(); @@ -142,10 +144,6 @@ ->assertTableActionEnabled(DeleteAction::class, $user) ->callTableAction(DeleteAction::class, $user); - // $this->assertModelMissing($user); - - // replaced the above line with the following because this properly checks that the user is soft deleted instead of completely removed from the database. - $this->assertSoftDeleted('users', ['id' => $user->id]); }); diff --git a/tests/Feature/UserTrashRestoreTest.php b/tests/Feature/UserTrashRestoreTest.php index e6de507..acd799b 100644 --- a/tests/Feature/UserTrashRestoreTest.php +++ b/tests/Feature/UserTrashRestoreTest.php @@ -3,7 +3,8 @@ use Eclipse\Core\Models\User; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\Auth; -use Spatie\Permission\Exceptions\UnauthorizedException; +use Illuminate\Support\Facades\Gate; +use Illuminate\Auth\Access\AuthorizationException; use Spatie\Permission\Models\Permission; uses(RefreshDatabase::class); @@ -12,10 +13,25 @@ $this->set_up_super_admin_and_tenant(); $this->admin = auth()->user(); - Permission::firstOrCreate(['name' => 'delete users']); - Permission::firstOrCreate(['name' => 'restore users']); + $shieldPermissions = [ + 'view_any_user', + 'view_user', + 'create_user', + 'update_user', + 'delete_user', + 'delete_any_user', + 'restore_user', + 'restore_any_user', + 'force_delete_user', + 'force_delete_any_user', + ]; + + foreach ($shieldPermissions as $permission) { + Permission::firstOrCreate(['name' => $permission]); + } - $this->admin->syncPermissions(['delete users', 'restore users']); + // Assign all permissions to admin for testing + $this->admin->syncPermissions($shieldPermissions); }); test('authorized user with permission can trash another user', function () { @@ -23,69 +39,198 @@ Auth::login($this->admin); - expect($this->admin->can('delete users'))->toBeTrue(); + // Verify the user has the Shield-generated permission + $this->assertTrue($this->admin->hasPermissionTo('delete_user')); + + // Verify permission check through policy + $this->assertTrue($this->admin->can('delete', $user)); $user->delete(); - expect($user->fresh()->trashed())->toBeTrue(); + $this->assertTrue($user->fresh()->trashed()); }); test('non-authorized user cannot trash another user', function () { $user = User::factory()->create(); - $otherUser = User::factory()->create(); - + $targetUser = User::factory()->create(); + + // User without Shield permissions Auth::login($user); - - expect($user->can('delete users'))->toBeFalse(); - - $this->expectException(UnauthorizedException::class); - $otherUser->delete(); + // Verify user doesn't have the Shield-generated permission + $this->assertFalse($user->hasPermissionTo('delete_user')); + + // Verify policy check fails + $this->assertFalse($user->can('delete', $targetUser)); + + $this->expectException(AuthorizationException::class); + Gate::authorize('delete', $targetUser); }); test('user cannot trash himself', function () { Auth::login($this->admin); - - $this->expectException(\Exception::class); - - $this->admin->delete(); + + // Even with permissions, admin shouldn't be able to delete themselves + $this->assertFalse($this->admin->can('delete', $this->admin)); + + // Try to delete and expect an exception + try { + // Attempt to authorize the action (should fail) + Gate::authorize('delete', $this->admin); + $this->fail('User was able to authorize self-deletion, which should not be allowed'); + } catch (AuthorizationException $e) { + // Expected behavior + $this->assertTrue(true); + } + + // Verify admin still exists and is not trashed + $this->assertFalse($this->admin->fresh()->trashed()); }); -test('authorized user with permission can restore a trashed user', function () { +test('authorized user with restore permission can restore a trashed user', function () { $user = User::factory()->create(); - $user->delete(); + $user->delete(); // Trashing the user Auth::login($this->admin); - expect($this->admin->can('restore users'))->toBeTrue(); + // Verify the user has the Shield-generated permission + $this->assertTrue($this->admin->hasPermissionTo('restore_user')); + + // Check policy integration + $this->assertTrue($this->admin->can('restore', $user)); + // Restore the trashed user $user->restore(); - expect($user->fresh()->trashed())->toBeFalse(); + // Assert the user is no longer trashed + $this->assertFalse($user->fresh()->trashed()); +}); + +test('authorized user with restore_any permission can restore any trashed user', function () { + // Create and trash a regular user + $userToTrash = User::factory()->create(); + $userToTrash->delete(); // Trash the user + + // Create another admin with restore_any permission only + $limitedAdmin = User::factory()->create(); + + // Give this admin only the restore_any permission using Shield convention + $limitedAdmin->givePermissionTo('restore_any_user'); + + Auth::login($limitedAdmin); + + // First check - directly checking Shield permission + $this->assertTrue($limitedAdmin->hasPermissionTo('restore_any_user')); + + // Second check - checking policy gate (should use Shield's generated policy) + $this->assertTrue($limitedAdmin->can('restoreAny', User::class)); + + // Now restore the user + $userToTrash->restore(); + + // Check the user is no longer trashed + $this->assertFalse($userToTrash->fresh()->trashed()); }); test('non-authorized user cannot restore another user', function () { $userToTrash = User::factory()->create(); - $userToTrash->delete(); + $userToTrash->delete(); // Trashing the user $nonAuthorizedUser = User::factory()->create(); Auth::login($nonAuthorizedUser); - expect($nonAuthorizedUser->can('restore users'))->toBeFalse(); + // Verify user doesn't have Shield permission + $this->assertFalse($nonAuthorizedUser->hasPermissionTo('restore_user')); + + // Verify policy check fails + $this->assertFalse($nonAuthorizedUser->can('restore', $userToTrash)); + + $this->expectException(AuthorizationException::class); + Gate::authorize('restore', $userToTrash); +}); + +test('trashed user cannot login', function () { + $userToTrash = User::factory()->create([ + 'email' => 'trashed@example.com', + 'password' => bcrypt('password') + ]); + $userToTrash->delete(); // Trashing the user + + Auth::logout(); - $this->expectException(UnauthorizedException::class); + // Attempt to log in with the trashed user's credentials + $attempt = Auth::attempt([ + 'email' => 'trashed@example.com', + 'password' => 'password' + ]); - $userToTrash->restore(); + // Assert that login attempt fails for trashed user + $this->assertFalse($attempt); }); -test('trashed user cannot login', function () { +test('authorized user with permission can force delete a trashed user', function () { + $user = User::factory()->create(); + $user->delete(); // Trash first + + Auth::login($this->admin); + + // Verify the user has the Shield-generated permission + $this->assertTrue($this->admin->hasPermissionTo('force_delete_user')); + + // Check policy integration + $this->assertTrue($this->admin->can('forceDelete', $user)); + + $user->forceDelete(); + + $this->assertNull(User::withTrashed()->find($user->id)); +}); + +test('non-authorized user cannot force delete a trashed user', function () { $userToTrash = User::factory()->create(); - $userToTrash->delete(); + $userToTrash->delete(); // Trash first - Auth::logout(); + $nonAuthorizedUser = User::factory()->create(); + Auth::login($nonAuthorizedUser); + + // Verify user doesn't have Shield permission + $this->assertFalse($nonAuthorizedUser->hasPermissionTo('force_delete_user')); + + // Verify policy check fails + $this->assertFalse($nonAuthorizedUser->can('forceDelete', $userToTrash)); + + $this->expectException(AuthorizationException::class); + Gate::authorize('forceDelete', $userToTrash); +}); - $attempt = Auth::attempt(['email' => $userToTrash->email, 'password' => 'password']); +test('can view trashed users when user has permissions', function () { + $trashedUser = User::factory()->create(); + $trashedUser->delete(); + + Auth::login($this->admin); - expect($attempt)->toBeFalse(); + // Verify the user has the Shield-generated permissions + $this->assertTrue($this->admin->hasPermissionTo('view_any_user')); + $this->assertTrue($this->admin->hasPermissionTo('view_user')); + + // Check policy integration + $this->assertTrue($this->admin->can('viewAny', User::class)); + $this->assertTrue($this->admin->can('view', $trashedUser)); +}); + +test('filament resource can handle trashed users', function () { + // Create and trash a user + $userToTrash = User::factory()->create([ + 'name' => 'Trashed User', + 'email' => 'trashed@example.com' + ]); + $userToTrash->delete(); + + Auth::login($this->admin); + + // Verify admin can see trashed users + $this->assertTrue($this->admin->can('viewAny', User::class)); + + $this->assertNotNull(User::withTrashed()->where('email', 'trashed@example.com')->first()); + $this->assertTrue(User::withTrashed()->where('email', 'trashed@example.com')->first()->trashed()); }); \ No newline at end of file From b4adfe9910f61e6c37f99f04504b521455f7babf Mon Sep 17 00:00:00 2001 From: sinach Date: Thu, 20 Mar 2025 01:25:14 +0100 Subject: [PATCH 6/8] fix: refactored and fixed all issues regarding --- src/Filament/Resources/UserResource.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Filament/Resources/UserResource.php b/src/Filament/Resources/UserResource.php index 4521421..5d8a7b3 100644 --- a/src/Filament/Resources/UserResource.php +++ b/src/Filament/Resources/UserResource.php @@ -179,7 +179,7 @@ public static function table(Table $table): Table ->requiresConfirmation(), Tables\Actions\RestoreAction::make() ->visible(fn (User $user) => $user->trashed() && auth()->user()->can('restore_user')) - ->requiresConfirmation(), + ->requiresConfirmation() ->disabled(fn (User $user) => $user->id === auth()->user()->id), ]), ]) From 2d5d5753d76c61e50e63ebfd4f76fa37161daa38 Mon Sep 17 00:00:00 2001 From: sinach Date: Thu, 20 Mar 2025 01:30:06 +0100 Subject: [PATCH 7/8] fix: rewrote the user trash restore tests to use filament policies --- ..._15_000000_add_login_tracking_to_users.php | 3 +- src/EclipseServiceProvider.php | 19 +- src/Filament/Resources/UserResource.php | 14 +- src/Models/User.php | 26 +-- src/Policies/UserPolicy.php | 42 +++- .../Filament/Resources/UserResourceTest.php | 10 +- tests/Feature/UserTrashRestoreTest.php | 205 +++++++++++++++--- 7 files changed, 257 insertions(+), 62 deletions(-) diff --git a/database/migrations/2025_03_15_000000_add_login_tracking_to_users.php b/database/migrations/2025_03_15_000000_add_login_tracking_to_users.php index 103c22e..e6394a4 100644 --- a/database/migrations/2025_03_15_000000_add_login_tracking_to_users.php +++ b/database/migrations/2025_03_15_000000_add_login_tracking_to_users.php @@ -4,7 +4,8 @@ use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Facades\Schema; -return new class extends Migration { +return new class extends Migration +{ public function up() { Schema::table('users', function (Blueprint $table) { diff --git a/src/EclipseServiceProvider.php b/src/EclipseServiceProvider.php index 51f0f72..1ed7174 100644 --- a/src/EclipseServiceProvider.php +++ b/src/EclipseServiceProvider.php @@ -9,6 +9,8 @@ use Eclipse\Core\Providers\AdminPanelProvider; use Eclipse\Core\Providers\TelescopeServiceProvider; use Illuminate\Auth\Events\Login; +use Illuminate\Database\Eloquent\Model; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Event; use Spatie\LaravelPackageTools\Package; use Spatie\LaravelPackageTools\PackageServiceProvider; @@ -35,7 +37,7 @@ public function configurePackage(Package $package): void ->hasTranslations(); } - public function register() + public function register(): self { parent::register(); @@ -46,14 +48,25 @@ public function register() $event->user->updateLoginTracking(); } }); - + $this->app->register(AdminPanelProvider::class); - if ($this->app->environment('local') && class_exists(\Laravel\Telescope\TelescopeServiceProvider::class)) { + if ($this->app->environment('local')) { $this->app->register(\Laravel\Telescope\TelescopeServiceProvider::class); $this->app->register(TelescopeServiceProvider::class); } return $this; } + + public function boot(): void + { + parent::boot(); + + // Enable Model strictness when not in production + Model::shouldBeStrict(! app()->isProduction()); + + // Do not allow destructive DB commands in production + DB::prohibitDestructiveCommands(app()->isProduction()); + } } diff --git a/src/Filament/Resources/UserResource.php b/src/Filament/Resources/UserResource.php index 79796df..5d8a7b3 100644 --- a/src/Filament/Resources/UserResource.php +++ b/src/Filament/Resources/UserResource.php @@ -175,13 +175,12 @@ public static function table(Table $table): Table Tables\Actions\ViewAction::make(), Tables\Actions\EditAction::make(), Tables\Actions\DeleteAction::make() - ->authorize(fn () => auth()->user()?->hasPermissionTo('delete users')) - ->visible(fn () => auth()->user()?->hasPermissionTo('delete users')) - ->requiresConfirmation(), + ->authorize(fn (User $record) => auth()->user()->can('delete_user') && auth()->id() !== $record->id) + ->requiresConfirmation(), Tables\Actions\RestoreAction::make() - ->visible(fn (User $user) => $user->trashed()) - ->authorize(fn () => auth()->user()->can('restore users', User::class)) - ->requiresConfirmation(), + ->visible(fn (User $user) => $user->trashed() && auth()->user()->can('restore_user')) + ->requiresConfirmation() + ->disabled(fn (User $user) => $user->id === auth()->user()->id), ]), ]) ->bulkActions([ @@ -294,6 +293,9 @@ public static function getPermissionPrefixes(): array 'delete', 'delete_any', 'restore', + 'restore_any', + 'force_delete', + 'force_delete_any', ]; } } diff --git a/src/Models/User.php b/src/Models/User.php index 9f52ef6..d3251e5 100644 --- a/src/Models/User.php +++ b/src/Models/User.php @@ -144,13 +144,7 @@ public function updateLoginTracking() */ public function delete(): ?bool { - $authUser = auth()->user(); - - if (!$authUser || !$authUser->hasAnyRole(['super-admin']) && !$authUser->hasPermissionTo('delete users')) { - throw new UnauthorizedException(403, 'You do not have permission to delete users.'); - } - - if ($this->id === $authUser->id) { + if ($this->id === auth()->id()) { throw new \Exception('You cannot delete your own account.'); } @@ -159,12 +153,18 @@ public function delete(): ?bool public function restore(): bool { - $authUser = auth()->user(); - - if (!$authUser || !$authUser->hasAnyRole(['super-admin']) && !$authUser->hasPermissionTo('restore users')) { - throw new UnauthorizedException(403, 'You do not have permission to restore users.'); - } - return parent::restore(); } + + /** + * Update the user's last login timestamp and increment login count. + * + * @return void + */ + public function updateLoginTracking() + { + $this->last_login_at = now(); + $this->increment('login_count'); + $this->save(); + } } diff --git a/src/Policies/UserPolicy.php b/src/Policies/UserPolicy.php index b07f6b3..415dbb0 100644 --- a/src/Policies/UserPolicy.php +++ b/src/Policies/UserPolicy.php @@ -44,9 +44,13 @@ public function update(User $user): bool /** * Determine whether the user can delete the model. */ - public function delete(User $user): bool + public function delete(User $authenticatedUser, User $user): bool { - return $user->can('delete_user'); + if ($authenticatedUser->id === $user->id) { + return false; + } + + return $authenticatedUser->can('delete_user'); } /** @@ -56,4 +60,36 @@ public function deleteAny(User $user): bool { return $user->can('delete_any_user'); } -} + + /** + * Determine whether the user can restore the model. + */ + public function restore(User $user): bool + { + return $user->can('restore_user'); + } + + /** + * Determine whether the user can bulk restore. + */ + public function restoreAny(User $user): bool + { + return $user->can('restore_any_user'); + } + + /** + * Determine whether the user can permanently delete the model. + */ + public function forceDelete(User $user): bool + { + return $user->can('force_delete_user'); + } + + /** + * Determine whether the user can permanently bulk delete. + */ + public function forceDeleteAny(User $user): bool + { + return $user->can('force_delete_any_user'); + } +} \ No newline at end of file diff --git a/tests/Feature/Filament/Resources/UserResourceTest.php b/tests/Feature/Filament/Resources/UserResourceTest.php index b45d261..cee5cc1 100644 --- a/tests/Feature/Filament/Resources/UserResourceTest.php +++ b/tests/Feature/Filament/Resources/UserResourceTest.php @@ -14,9 +14,11 @@ beforeEach(function () { $this->set_up_super_admin_and_tenant(); - collect(['delete users', 'restore users'])->each(fn ($permission) => + collect(['delete_user', 'restore_user'])->each(fn ($permission) => Permission::firstOrCreate(['name' => $permission, 'guard_name' => 'web']) ); + + auth()->user()->syncPermissions(['delete_user', 'restore_user']); }); test('authorized access can be allowed', function () { @@ -132,7 +134,7 @@ $admin = auth()->user(); - $admin->syncPermissions(['delete users']); + $admin->syncPermissions(['delete_user']); $user = User::factory()->create(); @@ -142,10 +144,6 @@ ->assertTableActionEnabled(DeleteAction::class, $user) ->callTableAction(DeleteAction::class, $user); - // $this->assertModelMissing($user); - - // replaced the above line with the following because this properly checks that the user is soft deleted instead of completely removed from the database. - $this->assertSoftDeleted('users', ['id' => $user->id]); }); diff --git a/tests/Feature/UserTrashRestoreTest.php b/tests/Feature/UserTrashRestoreTest.php index e6de507..acd799b 100644 --- a/tests/Feature/UserTrashRestoreTest.php +++ b/tests/Feature/UserTrashRestoreTest.php @@ -3,7 +3,8 @@ use Eclipse\Core\Models\User; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\Auth; -use Spatie\Permission\Exceptions\UnauthorizedException; +use Illuminate\Support\Facades\Gate; +use Illuminate\Auth\Access\AuthorizationException; use Spatie\Permission\Models\Permission; uses(RefreshDatabase::class); @@ -12,10 +13,25 @@ $this->set_up_super_admin_and_tenant(); $this->admin = auth()->user(); - Permission::firstOrCreate(['name' => 'delete users']); - Permission::firstOrCreate(['name' => 'restore users']); + $shieldPermissions = [ + 'view_any_user', + 'view_user', + 'create_user', + 'update_user', + 'delete_user', + 'delete_any_user', + 'restore_user', + 'restore_any_user', + 'force_delete_user', + 'force_delete_any_user', + ]; + + foreach ($shieldPermissions as $permission) { + Permission::firstOrCreate(['name' => $permission]); + } - $this->admin->syncPermissions(['delete users', 'restore users']); + // Assign all permissions to admin for testing + $this->admin->syncPermissions($shieldPermissions); }); test('authorized user with permission can trash another user', function () { @@ -23,69 +39,198 @@ Auth::login($this->admin); - expect($this->admin->can('delete users'))->toBeTrue(); + // Verify the user has the Shield-generated permission + $this->assertTrue($this->admin->hasPermissionTo('delete_user')); + + // Verify permission check through policy + $this->assertTrue($this->admin->can('delete', $user)); $user->delete(); - expect($user->fresh()->trashed())->toBeTrue(); + $this->assertTrue($user->fresh()->trashed()); }); test('non-authorized user cannot trash another user', function () { $user = User::factory()->create(); - $otherUser = User::factory()->create(); - + $targetUser = User::factory()->create(); + + // User without Shield permissions Auth::login($user); - - expect($user->can('delete users'))->toBeFalse(); - - $this->expectException(UnauthorizedException::class); - $otherUser->delete(); + // Verify user doesn't have the Shield-generated permission + $this->assertFalse($user->hasPermissionTo('delete_user')); + + // Verify policy check fails + $this->assertFalse($user->can('delete', $targetUser)); + + $this->expectException(AuthorizationException::class); + Gate::authorize('delete', $targetUser); }); test('user cannot trash himself', function () { Auth::login($this->admin); - - $this->expectException(\Exception::class); - - $this->admin->delete(); + + // Even with permissions, admin shouldn't be able to delete themselves + $this->assertFalse($this->admin->can('delete', $this->admin)); + + // Try to delete and expect an exception + try { + // Attempt to authorize the action (should fail) + Gate::authorize('delete', $this->admin); + $this->fail('User was able to authorize self-deletion, which should not be allowed'); + } catch (AuthorizationException $e) { + // Expected behavior + $this->assertTrue(true); + } + + // Verify admin still exists and is not trashed + $this->assertFalse($this->admin->fresh()->trashed()); }); -test('authorized user with permission can restore a trashed user', function () { +test('authorized user with restore permission can restore a trashed user', function () { $user = User::factory()->create(); - $user->delete(); + $user->delete(); // Trashing the user Auth::login($this->admin); - expect($this->admin->can('restore users'))->toBeTrue(); + // Verify the user has the Shield-generated permission + $this->assertTrue($this->admin->hasPermissionTo('restore_user')); + + // Check policy integration + $this->assertTrue($this->admin->can('restore', $user)); + // Restore the trashed user $user->restore(); - expect($user->fresh()->trashed())->toBeFalse(); + // Assert the user is no longer trashed + $this->assertFalse($user->fresh()->trashed()); +}); + +test('authorized user with restore_any permission can restore any trashed user', function () { + // Create and trash a regular user + $userToTrash = User::factory()->create(); + $userToTrash->delete(); // Trash the user + + // Create another admin with restore_any permission only + $limitedAdmin = User::factory()->create(); + + // Give this admin only the restore_any permission using Shield convention + $limitedAdmin->givePermissionTo('restore_any_user'); + + Auth::login($limitedAdmin); + + // First check - directly checking Shield permission + $this->assertTrue($limitedAdmin->hasPermissionTo('restore_any_user')); + + // Second check - checking policy gate (should use Shield's generated policy) + $this->assertTrue($limitedAdmin->can('restoreAny', User::class)); + + // Now restore the user + $userToTrash->restore(); + + // Check the user is no longer trashed + $this->assertFalse($userToTrash->fresh()->trashed()); }); test('non-authorized user cannot restore another user', function () { $userToTrash = User::factory()->create(); - $userToTrash->delete(); + $userToTrash->delete(); // Trashing the user $nonAuthorizedUser = User::factory()->create(); Auth::login($nonAuthorizedUser); - expect($nonAuthorizedUser->can('restore users'))->toBeFalse(); + // Verify user doesn't have Shield permission + $this->assertFalse($nonAuthorizedUser->hasPermissionTo('restore_user')); + + // Verify policy check fails + $this->assertFalse($nonAuthorizedUser->can('restore', $userToTrash)); + + $this->expectException(AuthorizationException::class); + Gate::authorize('restore', $userToTrash); +}); + +test('trashed user cannot login', function () { + $userToTrash = User::factory()->create([ + 'email' => 'trashed@example.com', + 'password' => bcrypt('password') + ]); + $userToTrash->delete(); // Trashing the user + + Auth::logout(); - $this->expectException(UnauthorizedException::class); + // Attempt to log in with the trashed user's credentials + $attempt = Auth::attempt([ + 'email' => 'trashed@example.com', + 'password' => 'password' + ]); - $userToTrash->restore(); + // Assert that login attempt fails for trashed user + $this->assertFalse($attempt); }); -test('trashed user cannot login', function () { +test('authorized user with permission can force delete a trashed user', function () { + $user = User::factory()->create(); + $user->delete(); // Trash first + + Auth::login($this->admin); + + // Verify the user has the Shield-generated permission + $this->assertTrue($this->admin->hasPermissionTo('force_delete_user')); + + // Check policy integration + $this->assertTrue($this->admin->can('forceDelete', $user)); + + $user->forceDelete(); + + $this->assertNull(User::withTrashed()->find($user->id)); +}); + +test('non-authorized user cannot force delete a trashed user', function () { $userToTrash = User::factory()->create(); - $userToTrash->delete(); + $userToTrash->delete(); // Trash first - Auth::logout(); + $nonAuthorizedUser = User::factory()->create(); + Auth::login($nonAuthorizedUser); + + // Verify user doesn't have Shield permission + $this->assertFalse($nonAuthorizedUser->hasPermissionTo('force_delete_user')); + + // Verify policy check fails + $this->assertFalse($nonAuthorizedUser->can('forceDelete', $userToTrash)); + + $this->expectException(AuthorizationException::class); + Gate::authorize('forceDelete', $userToTrash); +}); - $attempt = Auth::attempt(['email' => $userToTrash->email, 'password' => 'password']); +test('can view trashed users when user has permissions', function () { + $trashedUser = User::factory()->create(); + $trashedUser->delete(); + + Auth::login($this->admin); - expect($attempt)->toBeFalse(); + // Verify the user has the Shield-generated permissions + $this->assertTrue($this->admin->hasPermissionTo('view_any_user')); + $this->assertTrue($this->admin->hasPermissionTo('view_user')); + + // Check policy integration + $this->assertTrue($this->admin->can('viewAny', User::class)); + $this->assertTrue($this->admin->can('view', $trashedUser)); +}); + +test('filament resource can handle trashed users', function () { + // Create and trash a user + $userToTrash = User::factory()->create([ + 'name' => 'Trashed User', + 'email' => 'trashed@example.com' + ]); + $userToTrash->delete(); + + Auth::login($this->admin); + + // Verify admin can see trashed users + $this->assertTrue($this->admin->can('viewAny', User::class)); + + $this->assertNotNull(User::withTrashed()->where('email', 'trashed@example.com')->first()); + $this->assertTrue(User::withTrashed()->where('email', 'trashed@example.com')->first()->trashed()); }); \ No newline at end of file From 66c6de6f1829705f03810112fb6590fd9a8f4cff Mon Sep 17 00:00:00 2001 From: sinach Date: Thu, 20 Mar 2025 16:04:40 +0100 Subject: [PATCH 8/8] fix: made all fixes to user trash restore --- src/Filament/Resources/UserResource.php | 24 +-- src/Models/User.php | 1 - .../Filament/Resources/UserResourceTest.php | 19 +-- tests/Feature/UserTrashRestoreTest.php | 158 ++++-------------- 4 files changed, 46 insertions(+), 156 deletions(-) diff --git a/src/Filament/Resources/UserResource.php b/src/Filament/Resources/UserResource.php index 5d8a7b3..13b158d 100644 --- a/src/Filament/Resources/UserResource.php +++ b/src/Filament/Resources/UserResource.php @@ -20,6 +20,7 @@ use Filament\Tables\Filters\QueryBuilder\Constraints\TextConstraint; use Filament\Tables\Table; use Illuminate\Database\Eloquent\Builder; +use Illuminate\Database\Eloquent\SoftDeletingScope; use Illuminate\Support\Facades\Hash; class UserResource extends Resource implements HasShieldPermissions @@ -32,8 +33,6 @@ class UserResource extends Resource implements HasShieldPermissions protected static ?string $recordTitleAttribute = 'first_name'; - protected static bool $softDelete = true; - public static function form(Form $form): Form { return $form->schema([ @@ -162,9 +161,7 @@ public static function table(Table $table): Table TextConstraint::make('login_count') ->label('Total Logins'), ]), - - // added trash filter - Tables\Filters\TrashedFilter::make() + Tables\Filters\TrashedFilter::make() ]; return $table @@ -175,12 +172,11 @@ public static function table(Table $table): Table Tables\Actions\ViewAction::make(), Tables\Actions\EditAction::make(), Tables\Actions\DeleteAction::make() - ->authorize(fn (User $record) => auth()->user()->can('delete_user') && auth()->id() !== $record->id) - ->requiresConfirmation(), + ->authorize(fn (User $record) => auth()->user()->can('delete_user') && auth()->id() !== $record->id) + ->requiresConfirmation(), Tables\Actions\RestoreAction::make() - ->visible(fn (User $user) => $user->trashed() && auth()->user()->can('restore_user')) - ->requiresConfirmation() - ->disabled(fn (User $user) => $user->id === auth()->user()->id), + ->visible(fn (User $user) => $user->trashed() && auth()->user()->can('restore_user')) + ->requiresConfirmation(), ]), ]) ->bulkActions([ @@ -283,6 +279,14 @@ public static function getGloballySearchableAttributes(): array ]; } + public static function getEloquentQuery(): Builder + { + return parent::getEloquentQuery() + ->withoutGlobalScopes([ + SoftDeletingScope::class, + ]); + } + public static function getPermissionPrefixes(): array { return [ diff --git a/src/Models/User.php b/src/Models/User.php index 8375256..a7f5dab 100644 --- a/src/Models/User.php +++ b/src/Models/User.php @@ -16,7 +16,6 @@ use Spatie\MediaLibrary\InteractsWithMedia; use Spatie\Permission\Traits\HasRoles; use Illuminate\Database\Eloquent\SoftDeletes; -use Spatie\Permission\Exceptions\UnauthorizedException; /** * @property int $id diff --git a/tests/Feature/Filament/Resources/UserResourceTest.php b/tests/Feature/Filament/Resources/UserResourceTest.php index cee5cc1..54b3c0f 100644 --- a/tests/Feature/Filament/Resources/UserResourceTest.php +++ b/tests/Feature/Filament/Resources/UserResourceTest.php @@ -13,12 +13,6 @@ beforeEach(function () { $this->set_up_super_admin_and_tenant(); - - collect(['delete_user', 'restore_user'])->each(fn ($permission) => - Permission::firstOrCreate(['name' => $permission, 'guard_name' => 'web']) - ); - - auth()->user()->syncPermissions(['delete_user', 'restore_user']); }); test('authorized access can be allowed', function () { @@ -130,12 +124,6 @@ }); test('user can be deleted', function () { - $this->set_up_super_admin_and_tenant(); - - $admin = auth()->user(); - - $admin->syncPermissions(['delete_user']); - $user = User::factory()->create(); livewire(ListUsers::class) @@ -148,13 +136,14 @@ }); test('authed user cannot delete himself', function () { + $superAdmin = User::withTrashed()->find($this->superAdmin->id); // Assert on table row action livewire(ListUsers::class) - ->assertTableActionDisabled(DeleteAction::class, $this->superAdmin); + ->assertTableActionDisabled(DeleteAction::class, $superAdmin); // Assert on bulk delete - $users = User::all(); + $users = User::all(); livewire(ListUsers::class) ->callTableBulkAction(DeleteBulkAction::class, $users) @@ -163,4 +152,4 @@ foreach ($users as $user) { $this->assertModelExists($user); } -}); +}); \ No newline at end of file diff --git a/tests/Feature/UserTrashRestoreTest.php b/tests/Feature/UserTrashRestoreTest.php index acd799b..3258721 100644 --- a/tests/Feature/UserTrashRestoreTest.php +++ b/tests/Feature/UserTrashRestoreTest.php @@ -5,147 +5,73 @@ use Illuminate\Support\Facades\Auth; use Illuminate\Support\Facades\Gate; use Illuminate\Auth\Access\AuthorizationException; -use Spatie\Permission\Models\Permission; uses(RefreshDatabase::class); beforeEach(function () { $this->set_up_super_admin_and_tenant(); - $this->admin = auth()->user(); - - $shieldPermissions = [ - 'view_any_user', - 'view_user', - 'create_user', - 'update_user', - 'delete_user', - 'delete_any_user', - 'restore_user', - 'restore_any_user', - 'force_delete_user', - 'force_delete_any_user', - ]; - - foreach ($shieldPermissions as $permission) { - Permission::firstOrCreate(['name' => $permission]); - } - - // Assign all permissions to admin for testing - $this->admin->syncPermissions($shieldPermissions); }); test('authorized user with permission can trash another user', function () { $user = User::factory()->create(); - - Auth::login($this->admin); - - // Verify the user has the Shield-generated permission - $this->assertTrue($this->admin->hasPermissionTo('delete_user')); - - // Verify permission check through policy - $this->assertTrue($this->admin->can('delete', $user)); - + Auth::login($this->superAdmin); + $this->assertTrue($this->superAdmin->hasPermissionTo('delete_user')); + $this->assertTrue($this->superAdmin->can('delete', $user)); $user->delete(); - $this->assertTrue($user->fresh()->trashed()); }); test('non-authorized user cannot trash another user', function () { $user = User::factory()->create(); $targetUser = User::factory()->create(); - - // User without Shield permissions Auth::login($user); - - // Verify user doesn't have the Shield-generated permission $this->assertFalse($user->hasPermissionTo('delete_user')); - - // Verify policy check fails $this->assertFalse($user->can('delete', $targetUser)); - $this->expectException(AuthorizationException::class); Gate::authorize('delete', $targetUser); }); test('user cannot trash himself', function () { - Auth::login($this->admin); - - // Even with permissions, admin shouldn't be able to delete themselves - $this->assertFalse($this->admin->can('delete', $this->admin)); - - // Try to delete and expect an exception + Auth::login($this->superAdmin); + $this->assertFalse($this->superAdmin->can('delete', $this->superAdmin)); try { - // Attempt to authorize the action (should fail) - Gate::authorize('delete', $this->admin); + Gate::authorize('delete', $this->superAdmin); $this->fail('User was able to authorize self-deletion, which should not be allowed'); } catch (AuthorizationException $e) { - // Expected behavior $this->assertTrue(true); } - - // Verify admin still exists and is not trashed - $this->assertFalse($this->admin->fresh()->trashed()); + $this->assertFalse($this->superAdmin->fresh()->trashed()); }); test('authorized user with restore permission can restore a trashed user', function () { $user = User::factory()->create(); - $user->delete(); // Trashing the user - - Auth::login($this->admin); - - // Verify the user has the Shield-generated permission - $this->assertTrue($this->admin->hasPermissionTo('restore_user')); - - // Check policy integration - $this->assertTrue($this->admin->can('restore', $user)); - - // Restore the trashed user + $user->delete(); + Auth::login($this->superAdmin); + $this->assertTrue($this->superAdmin->hasPermissionTo('restore_user')); + $this->assertTrue($this->superAdmin->can('restore', $user)); $user->restore(); - - // Assert the user is no longer trashed $this->assertFalse($user->fresh()->trashed()); }); test('authorized user with restore_any permission can restore any trashed user', function () { - // Create and trash a regular user $userToTrash = User::factory()->create(); - $userToTrash->delete(); // Trash the user - - // Create another admin with restore_any permission only + $userToTrash->delete(); $limitedAdmin = User::factory()->create(); - - // Give this admin only the restore_any permission using Shield convention $limitedAdmin->givePermissionTo('restore_any_user'); - Auth::login($limitedAdmin); - - // First check - directly checking Shield permission $this->assertTrue($limitedAdmin->hasPermissionTo('restore_any_user')); - - // Second check - checking policy gate (should use Shield's generated policy) $this->assertTrue($limitedAdmin->can('restoreAny', User::class)); - - // Now restore the user $userToTrash->restore(); - - // Check the user is no longer trashed $this->assertFalse($userToTrash->fresh()->trashed()); }); test('non-authorized user cannot restore another user', function () { $userToTrash = User::factory()->create(); - $userToTrash->delete(); // Trashing the user - + $userToTrash->delete(); $nonAuthorizedUser = User::factory()->create(); - Auth::login($nonAuthorizedUser); - - // Verify user doesn't have Shield permission $this->assertFalse($nonAuthorizedUser->hasPermissionTo('restore_user')); - - // Verify policy check fails $this->assertFalse($nonAuthorizedUser->can('restore', $userToTrash)); - $this->expectException(AuthorizationException::class); Gate::authorize('restore', $userToTrash); }); @@ -155,50 +81,32 @@ 'email' => 'trashed@example.com', 'password' => bcrypt('password') ]); - $userToTrash->delete(); // Trashing the user - + $userToTrash->delete(); Auth::logout(); - - // Attempt to log in with the trashed user's credentials $attempt = Auth::attempt([ - 'email' => 'trashed@example.com', + 'email' => 'trashed@example.com', 'password' => 'password' ]); - - // Assert that login attempt fails for trashed user $this->assertFalse($attempt); }); test('authorized user with permission can force delete a trashed user', function () { $user = User::factory()->create(); - $user->delete(); // Trash first - - Auth::login($this->admin); - - // Verify the user has the Shield-generated permission - $this->assertTrue($this->admin->hasPermissionTo('force_delete_user')); - - // Check policy integration - $this->assertTrue($this->admin->can('forceDelete', $user)); - + $user->delete(); + Auth::login($this->superAdmin); + $this->assertTrue($this->superAdmin->hasPermissionTo('force_delete_user')); + $this->assertTrue($this->superAdmin->can('forceDelete', $user)); $user->forceDelete(); - $this->assertNull(User::withTrashed()->find($user->id)); }); test('non-authorized user cannot force delete a trashed user', function () { $userToTrash = User::factory()->create(); - $userToTrash->delete(); // Trash first - + $userToTrash->delete(); $nonAuthorizedUser = User::factory()->create(); Auth::login($nonAuthorizedUser); - - // Verify user doesn't have Shield permission $this->assertFalse($nonAuthorizedUser->hasPermissionTo('force_delete_user')); - - // Verify policy check fails $this->assertFalse($nonAuthorizedUser->can('forceDelete', $userToTrash)); - $this->expectException(AuthorizationException::class); Gate::authorize('forceDelete', $userToTrash); }); @@ -206,31 +114,21 @@ test('can view trashed users when user has permissions', function () { $trashedUser = User::factory()->create(); $trashedUser->delete(); - - Auth::login($this->admin); - - // Verify the user has the Shield-generated permissions - $this->assertTrue($this->admin->hasPermissionTo('view_any_user')); - $this->assertTrue($this->admin->hasPermissionTo('view_user')); - - // Check policy integration - $this->assertTrue($this->admin->can('viewAny', User::class)); - $this->assertTrue($this->admin->can('view', $trashedUser)); + Auth::login($this->superAdmin); + $this->assertTrue($this->superAdmin->hasPermissionTo('view_any_user')); + $this->assertTrue($this->superAdmin->hasPermissionTo('view_user')); + $this->assertTrue($this->superAdmin->can('viewAny', User::class)); + $this->assertTrue($this->superAdmin->can('view', $trashedUser)); }); -test('filament resource can handle trashed users', function () { - // Create and trash a user +test('filament resource can handle trashed users', function () { $userToTrash = User::factory()->create([ 'name' => 'Trashed User', 'email' => 'trashed@example.com' ]); $userToTrash->delete(); - - Auth::login($this->admin); - - // Verify admin can see trashed users - $this->assertTrue($this->admin->can('viewAny', User::class)); - + Auth::login($this->superAdmin); + $this->assertTrue($this->superAdmin->can('viewAny', User::class)); $this->assertNotNull(User::withTrashed()->where('email', 'trashed@example.com')->first()); $this->assertTrue(User::withTrashed()->where('email', 'trashed@example.com')->first()->trashed()); }); \ No newline at end of file