Skip to content

Commit bd026ef

Browse files
eiresendezeiresendez
committed
FOUR-31148: Fix group permission authorization
1 parent 6c89eae commit bd026ef

3 files changed

Lines changed: 182 additions & 22 deletions

File tree

‎ProcessMaker/Http/Controllers/Api/PermissionController.php‎

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,9 @@
44

55
use Illuminate\Http\Request;
66
use Illuminate\Support\Facades\Auth;
7-
use Illuminate\Support\Facades\Cache;
8-
use ProcessMaker\Events\PermissionChanged;
7+
use Illuminate\Validation\ValidationException;
98
use ProcessMaker\Events\PermissionUpdated;
109
use ProcessMaker\Http\Controllers\Controller;
11-
use ProcessMaker\Http\Resources\ApiCollection;
1210
use ProcessMaker\Models\Group;
1311
use ProcessMaker\Models\Permission;
1412
use ProcessMaker\Models\User;
@@ -30,7 +28,7 @@ class PermissionController extends Controller
3028
*
3129
* @param Request $request
3230
*
33-
* @return Response
31+
* @return \Illuminate\Support\Collection
3432
*/
3533
public function index(Request $request)
3634
{
@@ -44,7 +42,7 @@ public function index(Request $request)
4442
*
4543
* @param Request $request
4644
*
47-
* @return Response
45+
* @return \Illuminate\Http\Response
4846
*
4947
* @OA\Put(
5048
* path="/permissions",
@@ -82,8 +80,22 @@ public function index(Request $request)
8280
*/
8381
public function update(Request $request)
8482
{
83+
$request->validate([
84+
'user_id' => 'required_without:group_id|integer',
85+
'group_id' => 'required_without:user_id|integer',
86+
'permission_names' => 'nullable|array',
87+
]);
88+
89+
if ($request->filled('user_id') && $request->filled('group_id')) {
90+
throw ValidationException::withMessages([
91+
'user_id' => [__('The user_id field cannot be present when group_id is present.')],
92+
'group_id' => [__('The group_id field cannot be present when user_id is present.')],
93+
]);
94+
}
95+
8596
//Obtain the requested user or group
86-
if ($request->input('user_id')) {
97+
if ($request->filled('user_id')) {
98+
$this->authorize('edit-users');
8799
$entity = User::findOrFail($request->input('user_id'));
88100
// Obtain user old Permissions before save
89101
$originalPermissionNames = $entity->permissions()->pluck('name')->toArray();
@@ -98,14 +110,15 @@ public function update(Request $request)
98110
$entity->is_administrator = $isSettingToAdmin;
99111
$entity->save();
100112
}
101-
} elseif ($request->input('group_id')) {
113+
} elseif ($request->filled('group_id')) {
114+
$this->authorize('edit-groups');
102115
$entity = Group::findOrFail($request->input('group_id'));
103116
// Obtain group old Permissions before save
104117
$originalPermissionNames = $entity->permissions()->pluck('name')->toArray();
105118
}
106119

107120
// Obtain the requested permission names for that entity
108-
$requestPermissions = $request->input('permission_names');
121+
$requestPermissions = $request->input('permission_names') ?? [];
109122

110123
// Convert permission names into a collection of Permission models
111124
$permissions = Permission::whereIn('name', $requestPermissions)->get();
@@ -114,7 +127,7 @@ public function update(Request $request)
114127
PermissionUpdated::dispatch(
115128
$requestPermissions,
116129
$originalPermissionNames,
117-
$entity->is_administrator ?: false,
130+
$entity instanceof User ? $entity->is_administrator : false,
118131
$request->input('user_id'),
119132
$request->input('group_id')
120133
);

‎routes/api.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@
210210

211211
// Permissions
212212
Route::get('permissions', [PermissionController::class, 'index'])->name('permissions.index');
213-
Route::put('permissions', [PermissionController::class, 'update'])->name('permissions.update')->middleware('can:edit-users');
213+
Route::put('permissions', [PermissionController::class, 'update'])->name('permissions.update');
214214

215215
// Tenant Jobs Dashboard API
216216
Route::get('tenant-queues/tenants', [TenantQueueController::class, 'getTenants'])->name('tenant-queue.tenants');

‎tests/Feature/Api/PermissionsTest.php‎

Lines changed: 159 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,10 @@ class PermissionsTest extends TestCase
2222
{
2323
use RequestHelper;
2424

25+
private const PERMISSIONS_URL = '/permissions';
26+
27+
private const PROCESSES_URL = '/processes';
28+
2529
protected function withUserSetup()
2630
{
2731
$this->user->is_administrator = false;
@@ -59,10 +63,10 @@ public function testApiPermissions()
5963
'status' => 'ACTIVE',
6064
]);
6165

62-
$response = $this->apiCall('GET', '/processes');
66+
$response = $this->apiCall('GET', self::PROCESSES_URL);
6367
$response->assertStatus(200);
6468

65-
$response = $this->apiCall('GET', '/processes/' . $process->id);
69+
$response = $this->apiCall('GET', self::PROCESSES_URL . '/' . $process->id);
6670
$response->assertStatus(200);
6771

6872
$permission = Permission::byName('archive-processes');
@@ -74,7 +78,7 @@ public function testApiPermissions()
7478
// Invalidate permission cache to ensure changes take effect
7579
$this->user->invalidatePermissionCache();
7680

77-
$response = $this->apiCall('DELETE', '/processes/' . $process->id);
81+
$response = $this->apiCall('DELETE', self::PROCESSES_URL . '/' . $process->id);
7882
$response->assertStatus(403);
7983

8084
$this->user->permissions()->attach($permission->id);
@@ -84,7 +88,7 @@ public function testApiPermissions()
8488
// Invalidate permission cache to ensure the new permission takes effect
8589
$this->user->invalidatePermissionCache();
8690

87-
$response = $this->apiCall('DELETE', '/processes/' . $process->id);
91+
$response = $this->apiCall('DELETE', self::PROCESSES_URL . '/' . $process->id);
8892
$response->assertStatus(204);
8993
}
9094

@@ -97,7 +101,7 @@ public function testSetPermissionsForUser()
97101

98102
$testUser = User::factory()->create();
99103
$testPermission = Permission::factory()->create();
100-
$response = $this->apiCall('PUT', '/permissions', [
104+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
101105
'user_id' => $testUser->id,
102106
'permission_names' => [$testPermission->name],
103107
]);
@@ -109,6 +113,149 @@ public function testSetPermissionsForUser()
109113
$this->assertEquals($testUser->permissions->first()->id, $testPermission->id);
110114
}
111115

116+
public function testSetPermissionsForGroupWithInheritedEditGroupsPermission()
117+
{
118+
$this->user = User::factory()->create([
119+
'password' => Hash::make('password'),
120+
'is_administrator' => false,
121+
]);
122+
$this->initializePermissions(false);
123+
124+
$adminGroup = Group::factory()->create();
125+
$adminGroup->permissions()->attach(Permission::whereIn('name', [
126+
'view-groups',
127+
'create-groups',
128+
'edit-groups',
129+
'delete-groups',
130+
])->pluck('id'));
131+
132+
GroupMember::factory()->create([
133+
'group_id' => $adminGroup->id,
134+
'member_type' => User::class,
135+
'member_id' => $this->user->id,
136+
]);
137+
138+
$this->user->invalidatePermissionCache();
139+
140+
$targetGroup = Group::factory()->create();
141+
142+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
143+
'group_id' => $targetGroup->id,
144+
'permission_names' => ['view-groups', 'edit-groups'],
145+
]);
146+
147+
$response->assertStatus(204);
148+
$this->assertEqualsCanonicalizing(
149+
['view-groups', 'edit-groups'],
150+
$targetGroup->refresh()->permissions()->pluck('name')->toArray()
151+
);
152+
}
153+
154+
public function testSetPermissionsForUserRequiresEditUsersPermission()
155+
{
156+
$this->user = User::factory()->create([
157+
'password' => Hash::make('password'),
158+
'is_administrator' => false,
159+
]);
160+
$this->initializePermissions(false);
161+
162+
$adminGroup = Group::factory()->create();
163+
$adminGroup->permissions()->attach(Permission::byName('edit-groups')->id);
164+
165+
GroupMember::factory()->create([
166+
'group_id' => $adminGroup->id,
167+
'member_type' => User::class,
168+
'member_id' => $this->user->id,
169+
]);
170+
171+
$this->user->invalidatePermissionCache();
172+
173+
$targetUser = User::factory()->create();
174+
175+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
176+
'user_id' => $targetUser->id,
177+
'permission_names' => ['view-groups'],
178+
]);
179+
180+
$response->assertStatus(403);
181+
$this->assertFalse($targetUser->refresh()->permissions()->where('name', 'view-groups')->exists());
182+
}
183+
184+
public function testSetPermissionsForGroupRequiresEditGroupsPermission()
185+
{
186+
$this->user = User::factory()->create([
187+
'password' => Hash::make('password'),
188+
'is_administrator' => false,
189+
]);
190+
$this->initializePermissions(false);
191+
192+
$targetGroup = Group::factory()->create();
193+
194+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
195+
'group_id' => $targetGroup->id,
196+
'permission_names' => ['view-groups'],
197+
]);
198+
199+
$response->assertStatus(403);
200+
$this->assertCount(0, $targetGroup->refresh()->permissions);
201+
}
202+
203+
public function testUnauthorizedPermissionUpdatesDoNotExposeTargetExistence()
204+
{
205+
$this->user = User::factory()->create([
206+
'password' => Hash::make('password'),
207+
'is_administrator' => false,
208+
]);
209+
$this->initializePermissions(false);
210+
211+
$targetUser = User::factory()->create();
212+
$targetGroup = Group::factory()->create();
213+
214+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
215+
'user_id' => $targetUser->id,
216+
'permission_names' => ['view-groups'],
217+
]);
218+
$response->assertStatus(403);
219+
220+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
221+
'user_id' => $targetUser->id + 1000,
222+
'permission_names' => ['view-groups'],
223+
]);
224+
$response->assertStatus(403);
225+
226+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
227+
'group_id' => $targetGroup->id,
228+
'permission_names' => ['view-groups'],
229+
]);
230+
$response->assertStatus(403);
231+
232+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
233+
'group_id' => $targetGroup->id + 1000,
234+
'permission_names' => ['view-groups'],
235+
]);
236+
$response->assertStatus(403);
237+
}
238+
239+
public function testSetPermissionsRequiresExactlyOneTarget()
240+
{
241+
$targetUser = User::factory()->create();
242+
$targetGroup = Group::factory()->create();
243+
244+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
245+
'permission_names' => ['view-groups'],
246+
]);
247+
248+
$response->assertStatus(422);
249+
250+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
251+
'user_id' => $targetUser->id,
252+
'group_id' => $targetGroup->id,
253+
'permission_names' => ['view-groups'],
254+
]);
255+
256+
$response->assertStatus(422);
257+
}
258+
112259
public function testSetPermissionsViewProcessCatalogForUser()
113260
{
114261
$faker = Faker::create();
@@ -180,7 +327,7 @@ public function testCategoryPermission()
180327
// Invalidate permission cache to ensure the new permission takes effect
181328
$this->user->invalidatePermissionCache();
182329

183-
$response = $this->apiCall('PUT', $url, $attrs);
330+
$this->apiCall('PUT', $url, $attrs);
184331
$this->assertEquals('Test Category Update', $class::find($id)->name);
185332

186333
// test view permission
@@ -250,8 +397,8 @@ public function testSetPermissionsViewMyRequestForUser()
250397
public function testSetPermissionsViewMyRequestForUsersAndGroupCreated()
251398
{
252399
// Set up the users and groups
253-
$users = User::factory()->count(5)->create();
254-
$groups = Group::factory()->count(3)->create();
400+
User::factory()->count(5)->create();
401+
Group::factory()->count(3)->create();
255402

256403
// Run the seeder
257404
$this->seed(PermissionSeeder::class);
@@ -279,7 +426,7 @@ public function testAdministratorRoleAssignment()
279426
$this->user = $regularUser;
280427
$this->user->save();
281428

282-
$response = $this->apiCall('PUT', '/permissions', [
429+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
283430
'user_id' => $targetUser->id,
284431
'is_administrator' => true,
285432
'permission_names' => [],
@@ -292,7 +439,7 @@ public function testAdministratorRoleAssignment()
292439
$targetUser->is_administrator = true;
293440
$targetUser->save();
294441

295-
$response = $this->apiCall('PUT', '/permissions', [
442+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
296443
'user_id' => $targetUser->id,
297444
'is_administrator' => false,
298445
'permission_names' => [],
@@ -306,7 +453,7 @@ public function testAdministratorRoleAssignment()
306453
$this->user = $adminUser;
307454
$this->user->save();
308455

309-
$response = $this->apiCall('PUT', '/permissions', [
456+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
310457
'user_id' => $targetUser->id,
311458
'is_administrator' => false,
312459
'permission_names' => [],
@@ -317,7 +464,7 @@ public function testAdministratorRoleAssignment()
317464
$this->assertFalse($targetUser->is_administrator);
318465

319466
// Test 4: Admin can grant admin role
320-
$response = $this->apiCall('PUT', '/permissions', [
467+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
321468
'user_id' => $targetUser->id,
322469
'is_administrator' => true,
323470
'permission_names' => [],

0 commit comments

Comments
 (0)