From 03bd513910b3389e9e7ae116425e5606aca94252 Mon Sep 17 00:00:00 2001 From: Eleazar Resendez Date: Thu, 14 May 2026 13:25:47 -0600 Subject: [PATCH 1/2] FOUR-31148: Fix group permission authorization --- .../Controllers/Api/PermissionController.php | 31 +++- routes/api.php | 2 +- tests/Feature/Api/PermissionsTest.php | 173 ++++++++++++++++-- 3 files changed, 184 insertions(+), 22 deletions(-) diff --git a/ProcessMaker/Http/Controllers/Api/PermissionController.php b/ProcessMaker/Http/Controllers/Api/PermissionController.php index 5fed46c80b..98d34cc722 100644 --- a/ProcessMaker/Http/Controllers/Api/PermissionController.php +++ b/ProcessMaker/Http/Controllers/Api/PermissionController.php @@ -4,11 +4,9 @@ use Illuminate\Http\Request; use Illuminate\Support\Facades\Auth; -use Illuminate\Support\Facades\Cache; -use ProcessMaker\Events\PermissionChanged; +use Illuminate\Validation\ValidationException; use ProcessMaker\Events\PermissionUpdated; use ProcessMaker\Http\Controllers\Controller; -use ProcessMaker\Http\Resources\ApiCollection; use ProcessMaker\Models\Group; use ProcessMaker\Models\Permission; use ProcessMaker\Models\User; @@ -30,7 +28,7 @@ class PermissionController extends Controller * * @param Request $request * - * @return Response + * @return \Illuminate\Support\Collection */ public function index(Request $request) { @@ -44,7 +42,7 @@ public function index(Request $request) * * @param Request $request * - * @return Response + * @return \Illuminate\Http\Response * * @OA\Put( * path="/permissions", @@ -82,8 +80,22 @@ public function index(Request $request) */ public function update(Request $request) { + $request->validate([ + 'user_id' => 'required_without:group_id|integer', + 'group_id' => 'required_without:user_id|integer', + 'permission_names' => 'nullable|array', + ]); + + if ($request->filled('user_id') && $request->filled('group_id')) { + throw ValidationException::withMessages([ + 'user_id' => [__('The user_id field cannot be present when group_id is present.')], + 'group_id' => [__('The group_id field cannot be present when user_id is present.')], + ]); + } + //Obtain the requested user or group - if ($request->input('user_id')) { + if ($request->filled('user_id')) { + $this->authorize('edit-users'); $entity = User::findOrFail($request->input('user_id')); // Obtain user old Permissions before save $originalPermissionNames = $entity->permissions()->pluck('name')->toArray(); @@ -98,14 +110,15 @@ public function update(Request $request) $entity->is_administrator = $isSettingToAdmin; $entity->save(); } - } elseif ($request->input('group_id')) { + } elseif ($request->filled('group_id')) { + $this->authorize('edit-groups'); $entity = Group::findOrFail($request->input('group_id')); // Obtain group old Permissions before save $originalPermissionNames = $entity->permissions()->pluck('name')->toArray(); } // Obtain the requested permission names for that entity - $requestPermissions = $request->input('permission_names'); + $requestPermissions = $request->input('permission_names') ?? []; // Convert permission names into a collection of Permission models $permissions = Permission::whereIn('name', $requestPermissions)->get(); @@ -114,7 +127,7 @@ public function update(Request $request) PermissionUpdated::dispatch( $requestPermissions, $originalPermissionNames, - $entity->is_administrator ?: false, + $entity instanceof User ? $entity->is_administrator : false, $request->input('user_id'), $request->input('group_id') ); diff --git a/routes/api.php b/routes/api.php index 011ca9d41d..b3e7891063 100644 --- a/routes/api.php +++ b/routes/api.php @@ -210,7 +210,7 @@ // Permissions Route::get('permissions', [PermissionController::class, 'index'])->name('permissions.index'); - Route::put('permissions', [PermissionController::class, 'update'])->name('permissions.update')->middleware('can:edit-users'); + Route::put('permissions', [PermissionController::class, 'update'])->name('permissions.update'); // Tenant Jobs Dashboard API Route::get('tenant-queues/tenants', [TenantQueueController::class, 'getTenants'])->name('tenant-queue.tenants'); diff --git a/tests/Feature/Api/PermissionsTest.php b/tests/Feature/Api/PermissionsTest.php index 196ed18f3d..d75c7cc47d 100644 --- a/tests/Feature/Api/PermissionsTest.php +++ b/tests/Feature/Api/PermissionsTest.php @@ -22,6 +22,10 @@ class PermissionsTest extends TestCase { use RequestHelper; + private const PERMISSIONS_URL = '/permissions'; + + private const PROCESSES_URL = '/processes'; + protected function withUserSetup() { $this->user->is_administrator = false; @@ -59,10 +63,10 @@ public function testApiPermissions() 'status' => 'ACTIVE', ]); - $response = $this->apiCall('GET', '/processes'); + $response = $this->apiCall('GET', self::PROCESSES_URL); $response->assertStatus(200); - $response = $this->apiCall('GET', '/processes/' . $process->id); + $response = $this->apiCall('GET', self::PROCESSES_URL . '/' . $process->id); $response->assertStatus(200); $permission = Permission::byName('archive-processes'); @@ -74,7 +78,7 @@ public function testApiPermissions() // Invalidate permission cache to ensure changes take effect $this->user->invalidatePermissionCache(); - $response = $this->apiCall('DELETE', '/processes/' . $process->id); + $response = $this->apiCall('DELETE', self::PROCESSES_URL . '/' . $process->id); $response->assertStatus(403); $this->user->permissions()->attach($permission->id); @@ -84,7 +88,7 @@ public function testApiPermissions() // Invalidate permission cache to ensure the new permission takes effect $this->user->invalidatePermissionCache(); - $response = $this->apiCall('DELETE', '/processes/' . $process->id); + $response = $this->apiCall('DELETE', self::PROCESSES_URL . '/' . $process->id); $response->assertStatus(204); } @@ -97,7 +101,7 @@ public function testSetPermissionsForUser() $testUser = User::factory()->create(); $testPermission = Permission::factory()->create(); - $response = $this->apiCall('PUT', '/permissions', [ + $response = $this->apiCall('PUT', self::PERMISSIONS_URL, [ 'user_id' => $testUser->id, 'permission_names' => [$testPermission->name], ]); @@ -109,6 +113,151 @@ public function testSetPermissionsForUser() $this->assertEquals($testUser->permissions->first()->id, $testPermission->id); } + public function testSetPermissionsForGroupWithInheritedEditGroupsPermission() + { + $this->user = User::factory()->create([ + 'password' => Hash::make('password'), + 'is_administrator' => false, + ]); + $this->initializePermissions(false); + + $adminGroup = Group::factory()->create(); + $adminGroup->permissions()->attach(Permission::whereIn('name', [ + 'view-groups', + 'create-groups', + 'edit-groups', + 'delete-groups', + ])->pluck('id')); + + GroupMember::factory()->create([ + 'group_id' => $adminGroup->id, + 'member_type' => User::class, + 'member_id' => $this->user->id, + ]); + + $this->user->invalidatePermissionCache(); + + $targetGroup = Group::factory()->create(); + + $response = $this->apiCall('PUT', self::PERMISSIONS_URL, [ + 'group_id' => $targetGroup->id, + 'permission_names' => ['view-groups', 'edit-groups'], + ]); + + $response->assertStatus(204); + $this->assertEqualsCanonicalizing( + ['view-groups', 'edit-groups'], + $targetGroup->refresh()->permissions()->pluck('name')->toArray() + ); + } + + public function testSetPermissionsForUserRequiresEditUsersPermission() + { + $this->user = User::factory()->create([ + 'password' => Hash::make('password'), + 'is_administrator' => false, + ]); + $this->initializePermissions(false); + + $adminGroup = Group::factory()->create(); + $adminGroup->permissions()->attach(Permission::byName('edit-groups')->id); + + GroupMember::factory()->create([ + 'group_id' => $adminGroup->id, + 'member_type' => User::class, + 'member_id' => $this->user->id, + ]); + + $this->user->invalidatePermissionCache(); + + $targetUser = User::factory()->create(); + + $response = $this->apiCall('PUT', self::PERMISSIONS_URL, [ + 'user_id' => $targetUser->id, + 'permission_names' => ['view-groups'], + ]); + + $response->assertStatus(403); + $this->assertFalse($targetUser->refresh()->permissions()->where('name', 'view-groups')->exists()); + } + + public function testSetPermissionsForGroupRequiresEditGroupsPermission() + { + $this->user = User::factory()->create([ + 'password' => Hash::make('password'), + 'is_administrator' => false, + ]); + $this->initializePermissions(false); + + $targetGroup = Group::factory()->create(); + + $response = $this->apiCall('PUT', self::PERMISSIONS_URL, [ + 'group_id' => $targetGroup->id, + 'permission_names' => ['view-groups'], + ]); + + $response->assertStatus(403); + $this->assertCount(0, $targetGroup->refresh()->permissions); + } + + public function testUnauthorizedPermissionUpdatesDoNotExposeTargetExistence() + { + $this->user = User::factory()->create([ + 'password' => Hash::make('password'), + 'is_administrator' => false, + ]); + $this->initializePermissions(false); + + $targetUser = User::factory()->create(); + $targetGroup = Group::factory()->create(); + $missingUserId = User::max('id') + 1; + $missingGroupId = Group::max('id') + 1; + + $response = $this->apiCall('PUT', self::PERMISSIONS_URL, [ + 'user_id' => $targetUser->id, + 'permission_names' => ['view-groups'], + ]); + $response->assertStatus(403); + + $response = $this->apiCall('PUT', self::PERMISSIONS_URL, [ + 'user_id' => $missingUserId, + 'permission_names' => ['view-groups'], + ]); + $response->assertStatus(403); + + $response = $this->apiCall('PUT', self::PERMISSIONS_URL, [ + 'group_id' => $targetGroup->id, + 'permission_names' => ['view-groups'], + ]); + $response->assertStatus(403); + + $response = $this->apiCall('PUT', self::PERMISSIONS_URL, [ + 'group_id' => $missingGroupId, + 'permission_names' => ['view-groups'], + ]); + $response->assertStatus(403); + } + + public function testSetPermissionsRequiresExactlyOneTarget() + { + $targetUser = User::factory()->create(); + $targetGroup = Group::factory()->create(); + + $response = $this->apiCall('PUT', self::PERMISSIONS_URL, [ + 'permission_names' => ['view-groups'], + ]); + + $response->assertStatus(422); + + $response = $this->apiCall('PUT', self::PERMISSIONS_URL, [ + 'user_id' => $targetUser->id, + 'group_id' => $targetGroup->id, + 'permission_names' => ['view-groups'], + ]); + + $response->assertStatus(422); + } + public function testSetPermissionsViewProcessCatalogForUser() { $faker = Faker::create(); @@ -180,7 +329,7 @@ public function testCategoryPermission() // Invalidate permission cache to ensure the new permission takes effect $this->user->invalidatePermissionCache(); - $response = $this->apiCall('PUT', $url, $attrs); + $this->apiCall('PUT', $url, $attrs); $this->assertEquals('Test Category Update', $class::find($id)->name); // test view permission @@ -250,8 +399,8 @@ public function testSetPermissionsViewMyRequestForUser() public function testSetPermissionsViewMyRequestForUsersAndGroupCreated() { // Set up the users and groups - $users = User::factory()->count(5)->create(); - $groups = Group::factory()->count(3)->create(); + User::factory()->count(5)->create(); + Group::factory()->count(3)->create(); // Run the seeder $this->seed(PermissionSeeder::class); @@ -279,7 +428,7 @@ public function testAdministratorRoleAssignment() $this->user = $regularUser; $this->user->save(); - $response = $this->apiCall('PUT', '/permissions', [ + $response = $this->apiCall('PUT', self::PERMISSIONS_URL, [ 'user_id' => $targetUser->id, 'is_administrator' => true, 'permission_names' => [], @@ -292,7 +441,7 @@ public function testAdministratorRoleAssignment() $targetUser->is_administrator = true; $targetUser->save(); - $response = $this->apiCall('PUT', '/permissions', [ + $response = $this->apiCall('PUT', self::PERMISSIONS_URL, [ 'user_id' => $targetUser->id, 'is_administrator' => false, 'permission_names' => [], @@ -306,7 +455,7 @@ public function testAdministratorRoleAssignment() $this->user = $adminUser; $this->user->save(); - $response = $this->apiCall('PUT', '/permissions', [ + $response = $this->apiCall('PUT', self::PERMISSIONS_URL, [ 'user_id' => $targetUser->id, 'is_administrator' => false, 'permission_names' => [], @@ -317,7 +466,7 @@ public function testAdministratorRoleAssignment() $this->assertFalse($targetUser->is_administrator); // Test 4: Admin can grant admin role - $response = $this->apiCall('PUT', '/permissions', [ + $response = $this->apiCall('PUT', self::PERMISSIONS_URL, [ 'user_id' => $targetUser->id, 'is_administrator' => true, 'permission_names' => [], From f47bcea714674cf3a4b300f8d9aa5f3587cf6679 Mon Sep 17 00:00:00 2001 From: Eleazar Resendez Date: Mon, 18 May 2026 09:36:49 -0600 Subject: [PATCH 2/2] FOUR-31148: Add inherited edit-users permission test --- tests/Feature/Api/PermissionsTest.php | 33 +++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/Feature/Api/PermissionsTest.php b/tests/Feature/Api/PermissionsTest.php index d75c7cc47d..0ce4d4a1ba 100644 --- a/tests/Feature/Api/PermissionsTest.php +++ b/tests/Feature/Api/PermissionsTest.php @@ -151,6 +151,39 @@ public function testSetPermissionsForGroupWithInheritedEditGroupsPermission() ); } + public function testSetPermissionsForUserWithInheritedEditUsersPermission() + { + $this->user = User::factory()->create([ + 'password' => Hash::make('password'), + 'is_administrator' => false, + ]); + $this->initializePermissions(false); + + $adminGroup = Group::factory()->create(); + $adminGroup->permissions()->attach(Permission::byName('edit-users')->id); + + GroupMember::factory()->create([ + 'group_id' => $adminGroup->id, + 'member_type' => User::class, + 'member_id' => $this->user->id, + ]); + + $this->user->invalidatePermissionCache(); + + $targetUser = User::factory()->create(); + + $response = $this->apiCall('PUT', self::PERMISSIONS_URL, [ + 'user_id' => $targetUser->id, + 'permission_names' => ['view-groups'], + ]); + + $response->assertStatus(204); + $this->assertEqualsCanonicalizing( + ['view-groups'], + $targetUser->refresh()->permissions()->pluck('name')->toArray() + ); + } + public function testSetPermissionsForUserRequiresEditUsersPermission() { $this->user = User::factory()->create([