From 234fa0632411adf30b70f6d051ab9b765be465fa Mon Sep 17 00:00:00 2001 From: Constantin Graf Date: Tue, 9 Apr 2024 13:06:56 +0200 Subject: [PATCH] Fixed permissions for tasks endpoints --- .../Controllers/Api/V1/ProjectController.php | 8 +- .../Controllers/Api/V1/TaskController.php | 14 +++ .../Requests/V1/Task/TaskIndexRequest.php | 10 ++- app/Models/Project.php | 15 ++++ app/Providers/JetstreamServiceProvider.php | 4 + tests/TestCase.php | 6 ++ .../Unit/Endpoint/Api/V1/TaskEndpointTest.php | 89 ++++++++++++++++++- tests/Unit/Model/ProjectModelTest.php | 26 ++++++ 8 files changed, 163 insertions(+), 9 deletions(-) diff --git a/app/Http/Controllers/Api/V1/ProjectController.php b/app/Http/Controllers/Api/V1/ProjectController.php index 63d2ff16..e4d292c1 100644 --- a/app/Http/Controllers/Api/V1/ProjectController.php +++ b/app/Http/Controllers/Api/V1/ProjectController.php @@ -12,7 +12,6 @@ use App\Models\Organization; use App\Models\Project; use App\Models\User; use Illuminate\Auth\Access\AuthorizationException; -use Illuminate\Database\Eloquent\Builder; use Illuminate\Http\JsonResponse; use Illuminate\Http\Resources\Json\JsonResource; use Illuminate\Support\Facades\Auth; @@ -47,12 +46,7 @@ class ProjectController extends Controller ->whereBelongsTo($organization, 'organization'); if (! $canViewAllProjects) { - $projectsQuery->where(function (Builder $builder) use ($user): Builder { - return $builder->where('is_public', '=', true) - ->orWhereHas('members', function (Builder $builder) use ($user): Builder { - return $builder->whereBelongsTo($user, 'user'); - }); - }); + $projectsQuery->visibleByUser($user); } $projects = $projectsQuery->paginate(); diff --git a/app/Http/Controllers/Api/V1/TaskController.php b/app/Http/Controllers/Api/V1/TaskController.php index dd068435..96e90d89 100644 --- a/app/Http/Controllers/Api/V1/TaskController.php +++ b/app/Http/Controllers/Api/V1/TaskController.php @@ -10,10 +10,14 @@ use App\Http\Requests\V1\Task\TaskUpdateRequest; use App\Http\Resources\V1\Task\TaskCollection; use App\Http\Resources\V1\Task\TaskResource; use App\Models\Organization; +use App\Models\Project; use App\Models\Task; +use App\Models\User; use Illuminate\Auth\Access\AuthorizationException; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Http\JsonResponse; use Illuminate\Http\Resources\Json\JsonResource; +use Illuminate\Support\Facades\Auth; class TaskController extends Controller { @@ -37,6 +41,9 @@ class TaskController extends Controller public function index(Organization $organization, TaskIndexRequest $request): TaskCollection { $this->checkPermission($organization, 'tasks:view'); + $canViewAllTasks = $this->hasPermission($organization, 'tasks:view:all'); + /** @var User $user */ + $user = Auth::user(); $projectId = $request->input('project_id'); @@ -47,6 +54,13 @@ class TaskController extends Controller $query->where('project_id', '=', $projectId); } + if (! $canViewAllTasks) { + $query->whereHas('project', function (Builder $builder) use ($user): void { + /** @var Builder $builder */ + $builder->visibleByUser($user); + }); + } + $tasks = $query->paginate(); return new TaskCollection($tasks); diff --git a/app/Http/Requests/V1/Task/TaskIndexRequest.php b/app/Http/Requests/V1/Task/TaskIndexRequest.php index a80b7c1d..8d3b7480 100644 --- a/app/Http/Requests/V1/Task/TaskIndexRequest.php +++ b/app/Http/Requests/V1/Task/TaskIndexRequest.php @@ -6,9 +6,11 @@ namespace App\Http\Requests\V1\Task; use App\Models\Organization; use App\Models\Project; +use App\Service\PermissionStore; use Illuminate\Contracts\Validation\ValidationRule; use Illuminate\Database\Eloquent\Builder; use Illuminate\Foundation\Http\FormRequest; +use Illuminate\Support\Facades\Auth; use Korridor\LaravelModelValidationRules\Rules\ExistsEloquent; /** @@ -28,7 +30,13 @@ class TaskIndexRequest extends FormRequest 'uuid', new ExistsEloquent(Project::class, null, function (Builder $builder): Builder { /** @var Builder $builder */ - return $builder->whereBelongsTo($this->organization, 'organization'); + $builder = $builder->whereBelongsTo($this->organization, 'organization'); + + if (! app(PermissionStore::class)->has($this->organization, 'tasks:view:all')) { + $builder = $builder->visibleByUser(Auth::user()); + } + + return $builder; }), ], ]; diff --git a/app/Models/Project.php b/app/Models/Project.php index 043d3172..0cb4e57f 100644 --- a/app/Models/Project.php +++ b/app/Models/Project.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace App\Models; use Database\Factories\ProjectFactory; +use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Collection; use Illuminate\Database\Eloquent\Concerns\HasUuids; use Illuminate\Database\Eloquent\Factories\HasFactory; @@ -23,6 +24,7 @@ use Illuminate\Database\Eloquent\Relations\HasMany; * @property-read Client|null $client * @property-read Collection $tasks * + * @method Builder visibleByUser(User $user) * @method static ProjectFactory factory() */ class Project extends Model @@ -71,4 +73,17 @@ class Project extends Model { return $this->hasMany(Task::class); } + + /** + * @param Builder $builder + */ + public function scopeVisibleByUser(Builder $builder, User $user): void + { + $builder->where(function (Builder $builder) use ($user): Builder { + return $builder->where('is_public', '=', true) + ->orWhereHas('members', function (Builder $builder) use ($user): Builder { + return $builder->whereBelongsTo($user, 'user'); + }); + }); + } } diff --git a/app/Providers/JetstreamServiceProvider.php b/app/Providers/JetstreamServiceProvider.php index b402b515..99444adc 100644 --- a/app/Providers/JetstreamServiceProvider.php +++ b/app/Providers/JetstreamServiceProvider.php @@ -119,6 +119,10 @@ class JetstreamServiceProvider extends ServiceProvider 'tags:create', 'tags:update', 'tags:delete', + 'clients:view', + 'clients:create', + 'clients:update', + 'clients:delete', 'organizations:view', 'members:view', ])->description('Managers have the ability to read, create, and update their own time entries as well as those of their team.'); diff --git a/tests/TestCase.php b/tests/TestCase.php index dbbd313d..e47e0a72 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Tests; +use Illuminate\Database\Eloquent\Collection; use Illuminate\Foundation\Testing\TestCase as BaseTestCase; use TiMacDonald\Log\LogFake; @@ -16,4 +17,9 @@ abstract class TestCase extends BaseTestCase parent::setUp(); LogFake::bind(); } + + protected function assertEqualsIdsOfEloquentCollection(array $ids, Collection $models): void + { + $this->assertEqualsCanonicalizing($ids, $models->pluck('id')->toArray()); + } } diff --git a/tests/Unit/Endpoint/Api/V1/TaskEndpointTest.php b/tests/Unit/Endpoint/Api/V1/TaskEndpointTest.php index a57fead4..7a3f8a59 100644 --- a/tests/Unit/Endpoint/Api/V1/TaskEndpointTest.php +++ b/tests/Unit/Endpoint/Api/V1/TaskEndpointTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Tests\Unit\Endpoint\Api\V1; use App\Models\Project; +use App\Models\ProjectMember; use App\Models\Task; use Laravel\Passport\Passport; @@ -25,11 +26,27 @@ class TaskEndpointTest extends ApiEndpointTestAbstract $response->assertForbidden(); } + public function test_index_endpoint_validation_fails_if_project_id_is_not_pat(): void + { + // Arrange + $data = $this->createUserWithPermission([ + ]); + Task::factory()->forOrganization($data->organization)->createMany(4); + Passport::actingAs($data->user); + + // Act + $response = $this->getJson(route('api.v1.tasks.index', [$data->organization->getKey()])); + + // Assert + $response->assertForbidden(); + } + public function test_index_endpoint_returns_list_of_all_tasks_of_organization(): void { // Arrange $data = $this->createUserWithPermission([ 'tasks:view', + 'tasks:view:all', ]); $tasks = Task::factory()->forOrganization($data->organization)->createMany(4); Passport::actingAs($data->user); @@ -42,11 +59,35 @@ class TaskEndpointTest extends ApiEndpointTestAbstract $response->assertJsonCount(4, 'data'); } + public function test_index_endpoint_returns_list_of_all_tasks_with_access_of_organization_if_user_has_no_all_permission(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'tasks:view', + ]); + $otherProject = Project::factory()->create(); + Task::factory()->forOrganization($data->organization)->forProject($otherProject)->createMany(4); + $projectPublic = Project::factory()->isPublic()->create(); + Task::factory()->forOrganization($data->organization)->forProject($projectPublic)->createMany(2); + $projectAsMember = Project::factory()->isPrivate()->create(); + ProjectMember::factory()->forProject($projectAsMember)->forUser($data->user)->create(); + Task::factory()->forOrganization($data->organization)->forProject($projectAsMember)->createMany(2); + Passport::actingAs($data->user); + + // Act + $response = $this->getJson(route('api.v1.tasks.index', [$data->organization->getKey()])); + + // Assert + $response->assertStatus(200); + $response->assertJsonCount(4, 'data'); + } + public function test_index_endpoint_returns_list_of_all_tasks_of_organization_filtered_by_project(): void { // Arrange $data = $this->createUserWithPermission([ 'tasks:view', + 'tasks:view:all', ]); $project = Project::factory()->forOrganization($data->organization)->create(); Task::factory()->forOrganization($data->organization)->createMany(4); @@ -90,7 +131,53 @@ class TaskEndpointTest extends ApiEndpointTestAbstract ]); } - public function test_store_endpoint_fails_if_user_has_no_permission_to_create_tags() + public function test_index_endpoint_validation_fails_if_project_is_not_visible_by_user_and_user_does_not_have_tasks_all_permission(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'tasks:view', + ]); + $project = Project::factory()->forOrganization($data->organization)->create(); + Task::factory()->forOrganization($data->organization)->createMany(4); + Passport::actingAs($data->user); + + // Act + $response = $this->getJson(route('api.v1.tasks.index', [ + $data->organization->getKey(), + 'project_id' => $project->getKey(), + ])); + + // Assert + $response->assertStatus(422); + $response->assertInvalid([ + 'project_id', + ]); + } + + public function test_index_endpoint_returns_list_of_all_tasks_of_organization_filtered_by_project_if_user_has_access_to_project(): void + { + // Arrange + $data = $this->createUserWithPermission([ + 'tasks:view', + ]); + $project = Project::factory()->forOrganization($data->organization)->create(); + ProjectMember::factory()->forProject($project)->forUser($data->user)->create(); + Task::factory()->forOrganization($data->organization)->createMany(4); + Task::factory()->forOrganization($data->organization)->forProject($project)->createMany(2); + Passport::actingAs($data->user); + + // Act + $response = $this->getJson(route('api.v1.tasks.index', [ + $data->organization->getKey(), + 'project_id' => $project->getKey(), + ])); + + // Assert + $response->assertStatus(200); + $response->assertJsonCount(2, 'data'); + } + + public function test_store_endpoint_fails_if_user_has_no_permission_to_create_tasks() { // Arrange $data = $this->createUserWithPermission([ diff --git a/tests/Unit/Model/ProjectModelTest.php b/tests/Unit/Model/ProjectModelTest.php index ab98fd9b..287adb68 100644 --- a/tests/Unit/Model/ProjectModelTest.php +++ b/tests/Unit/Model/ProjectModelTest.php @@ -9,6 +9,7 @@ use App\Models\Organization; use App\Models\Project; use App\Models\ProjectMember; use App\Models\Task; +use App\Models\User; class ProjectModelTest extends ModelTestAbstract { @@ -86,4 +87,29 @@ class ProjectModelTest extends ModelTestAbstract $this->assertCount(3, $membersRel); $this->assertTrue($membersRel->first()->is($members->first())); } + + public function test_scope_visible_by_user_filters_so_that_only_public_projects_or_projects_where_the_user_is_member_are_shown(): void + { + // Arrange + $user = User::factory()->create(); + $projectPrivate = Project::factory()->isPrivate()->create(); + $projectPublic = Project::factory()->isPublic()->create(); + $projectPrivateButMember = Project::factory()->isPrivate()->create(); + ProjectMember::factory()->forProject($projectPrivateButMember)->forUser($user)->create(); + + // Act + $projectsVisible = Project::query()->visibleByUser($user)->get(); + $allProjects = Project::query()->get(); + + // Assert + $this->assertEqualsIdsOfEloquentCollection([ + $projectPublic->getKey(), + $projectPrivateButMember->getKey(), + ], $projectsVisible); + $this->assertEqualsIdsOfEloquentCollection([ + $projectPrivate->getKey(), + $projectPublic->getKey(), + $projectPrivateButMember->getKey(), + ], $allProjects); + } }