From 267adf52cacadecd08e380622cffa3888be8643e Mon Sep 17 00:00:00 2001 From: Constantin Graf Date: Wed, 24 Apr 2024 15:07:39 +0200 Subject: [PATCH] Fixed permissions; Added tests for permission store --- app/Http/Middleware/ShareInertiaData.php | 3 +- app/Service/PermissionStore.php | 33 +++-- tests/Feature/DeleteAccountTest.php | 22 ++-- tests/TestCase.php | 8 ++ .../Endpoint/Api/V1/MemberEndpointTest.php | 5 +- tests/Unit/Service/PermissionStoreTest.php | 124 ++++++++++++++++++ 6 files changed, 170 insertions(+), 25 deletions(-) create mode 100644 tests/Unit/Service/PermissionStoreTest.php diff --git a/app/Http/Middleware/ShareInertiaData.php b/app/Http/Middleware/ShareInertiaData.php index 2f730733..92a1dcca 100644 --- a/app/Http/Middleware/ShareInertiaData.php +++ b/app/Http/Middleware/ShareInertiaData.php @@ -24,9 +24,10 @@ class ShareInertiaData */ public function handle(Request $request, Closure $next): Response { + /** @var PermissionStore $permissions */ $permissions = app(PermissionStore::class); Inertia::share(array_filter([ - 'permissions' => $request->user() !== null ? $permissions->permissions($request->user()->currentTeam) : [], + 'permissions' => $request->user() !== null && $request->user()->currentTeam !== null ? $permissions->getPermissions($request->user()->currentTeam) : [], 'jetstream' => function () use ($request) { /** @var User|null $user */ $user = $request->user(); diff --git a/app/Service/PermissionStore.php b/app/Service/PermissionStore.php index 070b44e8..7e1523f4 100644 --- a/app/Service/PermissionStore.php +++ b/app/Service/PermissionStore.php @@ -17,6 +17,11 @@ class PermissionStore */ private array $permissionCache = []; + public function clear(): void + { + $this->permissionCache = []; + } + public function has(Organization $organization, string $permission): bool { /** @var User|null $user */ @@ -26,15 +31,11 @@ class PermissionStore } if (! isset($this->permissionCache[$user->getKey().'|'.$organization->getKey()])) { - if ($user->ownsTeam($organization)) { - return true; - } - if (! $user->belongsToTeam($organization)) { return false; } - $permissions = $user->teamPermissions($organization); + $permissions = $this->getPermissionsByUser($organization, $user); $this->permissionCache[$user->getKey().'|'.$organization->getKey()] = $permissions; } else { $permissions = $this->permissionCache[$user->getKey().'|'.$organization->getKey()]; @@ -46,14 +47,8 @@ class PermissionStore /** * @return array */ - public function getPermissions(Organization $organization): array + private function getPermissionsByUser(Organization $organization, User $user): array { - /** @var User|null $user */ - $user = Auth::user(); - if ($user === null) { - return []; - } - if (! $user->belongsToTeam($organization)) { return []; } @@ -69,4 +64,18 @@ class PermissionStore return $role !== null ? ($roleObj?->permissions ?? []) : []; } + + /** + * @return array + */ + public function getPermissions(Organization $organization): array + { + /** @var User|null $user */ + $user = Auth::user(); + if ($user === null) { + return []; + } + + return $this->getPermissionsByUser($organization, $user); + } } diff --git a/tests/Feature/DeleteAccountTest.php b/tests/Feature/DeleteAccountTest.php index b75c4d6b..5bfc18d7 100644 --- a/tests/Feature/DeleteAccountTest.php +++ b/tests/Feature/DeleteAccountTest.php @@ -6,7 +6,6 @@ namespace Tests\Feature; use App\Models\User; use Illuminate\Foundation\Testing\RefreshDatabase; -use Laravel\Jetstream\Features; use Tests\TestCase; class DeleteAccountTest extends TestCase @@ -15,31 +14,32 @@ class DeleteAccountTest extends TestCase public function test_user_accounts_can_be_deleted(): void { - if (! Features::hasAccountDeletionFeatures()) { - $this->markTestSkipped('Account deletion is not enabled.'); - } - - $this->actingAs($user = User::factory()->create()); + // Arrange + $user = User::factory()->create(); + $this->actingAs($user); + // Act $response = $this->delete('/user', [ 'password' => 'password', ]); + // Assert + $response->assertStatus(302); $this->assertNull($user->fresh()); } public function test_correct_password_must_be_provided_before_account_can_be_deleted(): void { - if (! Features::hasAccountDeletionFeatures()) { - $this->markTestSkipped('Account deletion is not enabled.'); - } - - $this->actingAs($user = User::factory()->create()); + // Arrange + $user = User::factory()->create(); + $this->actingAs($user); + // Act $response = $this->delete('/user', [ 'password' => 'wrong-password', ]); + // Assert $this->assertNotNull($user->fresh()); } } diff --git a/tests/TestCase.php b/tests/TestCase.php index 0aeb8c23..e946ab2b 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Tests; +use App\Service\PermissionStore; use Illuminate\Database\Eloquent\Collection; use Illuminate\Foundation\Testing\TestCase as BaseTestCase; use Illuminate\Support\Facades\Mail; @@ -20,6 +21,13 @@ abstract class TestCase extends BaseTestCase LogFake::bind(); } + protected function tearDown(): void + { + // Note: It is necessary to clear the permission cache after each test, since the "scoped singletons" are not reset between tests. + app(PermissionStore::class)->clear(); + parent::tearDown(); + } + protected function assertEqualsIdsOfEloquentCollection(array $ids, Collection $models): void { $this->assertEqualsCanonicalizing($ids, $models->pluck('id')->toArray()); diff --git a/tests/Unit/Endpoint/Api/V1/MemberEndpointTest.php b/tests/Unit/Endpoint/Api/V1/MemberEndpointTest.php index b76caabe..045c6939 100644 --- a/tests/Unit/Endpoint/Api/V1/MemberEndpointTest.php +++ b/tests/Unit/Endpoint/Api/V1/MemberEndpointTest.php @@ -107,7 +107,8 @@ class MemberEndpointTest extends ApiEndpointTestAbstract { $data = $this->createUserWithPermission([ 'members:invite-placeholder', - ], true); + 'invitations:create', + ]); $user = User::factory()->create([ 'is_placeholder' => true, ]); @@ -242,6 +243,7 @@ class MemberEndpointTest extends ApiEndpointTestAbstract // Arrange $data = $this->createUserWithPermission([ 'members:invite-placeholder', + 'invitations:create', ]); $otherOrganization = Organization::factory()->create(); $user = User::factory()->create([ @@ -265,6 +267,7 @@ class MemberEndpointTest extends ApiEndpointTestAbstract // Arrange $data = $this->createUserWithPermission([ 'members:invite-placeholder', + 'invitations:create', ]); Passport::actingAs($data->user); diff --git a/tests/Unit/Service/PermissionStoreTest.php b/tests/Unit/Service/PermissionStoreTest.php new file mode 100644 index 00000000..7ed3154c --- /dev/null +++ b/tests/Unit/Service/PermissionStoreTest.php @@ -0,0 +1,124 @@ +create(); + $user = User::factory()->create(); + $organization->users()->attach($user, ['role' => 'employee']); + $permissionStore = new PermissionStore(); + + // Act + $result = $permissionStore->has($organization, 'permission'); + + // Assert + $this->assertFalse($result); + } + + public function test_has_method_returns_false_when_user_does_not_belong_to_organization(): void + { + // Arrange + $organization = Organization::factory()->create(); + $user = User::factory()->create(); + $permissionStore = new PermissionStore(); + $this->actingAs($user); + + // Act + $result = $permissionStore->has($organization, 'permission'); + + // Assert + $this->assertFalse($result); + } + + public function test_has_method_returns_false_when_user_does_not_have_permission(): void + { + // Arrange + $organization = Organization::factory()->create(); + $user = User::factory()->create(); + $organization->users()->attach($user, ['role' => 'employee']); + $permissionStore = new PermissionStore(); + $this->actingAs($user); + + // Act + $result = $permissionStore->has($organization, 'permission'); + + // Assert + $this->assertFalse($result); + } + + public function test_has_method_returns_true_when_user_has_permission(): void + { + // Arrange + $organization = Organization::factory()->create(); + $user = User::factory()->create(); + $organization->users()->attach($user, ['role' => 'employee']); + $permissionStore = new PermissionStore(); + $this->actingAs($user); + + // Act + $result = $permissionStore->has($organization, 'time-entries:view:own'); + + // Assert + $this->assertTrue($result); + } + + public function test_get_permissions_method_returns_empty_array_when_user_is_not_authenticated(): void + { + // Arrange + $organization = Organization::factory()->create(); + $user = User::factory()->create(); + $organization->users()->attach($user, ['role' => 'employee']); + $permissionStore = new PermissionStore(); + + // Act + $result = $permissionStore->getPermissions($organization); + + // Assert + $this->assertEmpty($result); + } + + public function test_get_permissions_method_returns_empty_array_when_user_does_not_belong_to_organization(): void + { + $organization = Organization::factory()->create(); + $user = User::factory()->create(); + $permissionStore = new PermissionStore(); + $this->actingAs($user); + + // Act + $result = $permissionStore->getPermissions($organization); + + // Assert + $this->assertEmpty($result); + } + + public function test_get_permissions_method_returns_permissions_when_user_belongs_to_organization(): void + { + // Arrange + $organization = Organization::factory()->create(); + $user = User::factory()->create(); + $organization->users()->attach($user, ['role' => 'employee']); + $permissionStore = new PermissionStore(); + $this->actingAs($user); + + // Act + $result = $permissionStore->getPermissions($organization); + + // Assert + $this->assertSame(Jetstream::findRole('employee')->permissions, $result); + } +}