Skip to content

Commit 4e8736d

Browse files
nolanpronolanpro
authored
Merge pull request #8827 from ProcessMaker/bugfix/FOUR-31148
FOUR-31148: Fix group permission authorization
2 parents c9da124 + 54f2484 commit 4e8736d

3 files changed

Lines changed: 217 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: 194 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,184 @@ 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 testSetPermissionsForUserWithInheritedEditUsersPermission()
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-users')->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(204);
181+
$this->assertEqualsCanonicalizing(
182+
['view-groups'],
183+
$targetUser->refresh()->permissions()->pluck('name')->toArray()
184+
);
185+
}
186+
187+
public function testSetPermissionsForUserRequiresEditUsersPermission()
188+
{
189+
$this->user = User::factory()->create([
190+
'password' => Hash::make('password'),
191+
'is_administrator' => false,
192+
]);
193+
$this->initializePermissions(false);
194+
195+
$adminGroup = Group::factory()->create();
196+
$adminGroup->permissions()->attach(Permission::byName('edit-groups')->id);
197+
198+
GroupMember::factory()->create([
199+
'group_id' => $adminGroup->id,
200+
'member_type' => User::class,
201+
'member_id' => $this->user->id,
202+
]);
203+
204+
$this->user->invalidatePermissionCache();
205+
206+
$targetUser = User::factory()->create();
207+
208+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
209+
'user_id' => $targetUser->id,
210+
'permission_names' => ['view-groups'],
211+
]);
212+
213+
$response->assertStatus(403);
214+
$this->assertFalse($targetUser->refresh()->permissions()->where('name', 'view-groups')->exists());
215+
}
216+
217+
public function testSetPermissionsForGroupRequiresEditGroupsPermission()
218+
{
219+
$this->user = User::factory()->create([
220+
'password' => Hash::make('password'),
221+
'is_administrator' => false,
222+
]);
223+
$this->initializePermissions(false);
224+
225+
$targetGroup = Group::factory()->create();
226+
227+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
228+
'group_id' => $targetGroup->id,
229+
'permission_names' => ['view-groups'],
230+
]);
231+
232+
$response->assertStatus(403);
233+
$this->assertCount(0, $targetGroup->refresh()->permissions);
234+
}
235+
236+
public function testUnauthorizedPermissionUpdatesDoNotExposeTargetExistence()
237+
{
238+
$this->user = User::factory()->create([
239+
'password' => Hash::make('password'),
240+
'is_administrator' => false,
241+
]);
242+
$this->initializePermissions(false);
243+
244+
$targetUser = User::factory()->create();
245+
$targetGroup = Group::factory()->create();
246+
$missingUserId = User::max('id') + 1;
247+
$missingGroupId = Group::max('id') + 1;
248+
249+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
250+
'user_id' => $targetUser->id,
251+
'permission_names' => ['view-groups'],
252+
]);
253+
$response->assertStatus(403);
254+
255+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
256+
'user_id' => $missingUserId,
257+
'permission_names' => ['view-groups'],
258+
]);
259+
$response->assertStatus(403);
260+
261+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
262+
'group_id' => $targetGroup->id,
263+
'permission_names' => ['view-groups'],
264+
]);
265+
$response->assertStatus(403);
266+
267+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
268+
'group_id' => $missingGroupId,
269+
'permission_names' => ['view-groups'],
270+
]);
271+
$response->assertStatus(403);
272+
}
273+
274+
public function testSetPermissionsRequiresExactlyOneTarget()
275+
{
276+
$targetUser = User::factory()->create();
277+
$targetGroup = Group::factory()->create();
278+
279+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
280+
'permission_names' => ['view-groups'],
281+
]);
282+
283+
$response->assertStatus(422);
284+
285+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
286+
'user_id' => $targetUser->id,
287+
'group_id' => $targetGroup->id,
288+
'permission_names' => ['view-groups'],
289+
]);
290+
291+
$response->assertStatus(422);
292+
}
293+
112294
public function testSetPermissionsViewProcessCatalogForUser()
113295
{
114296
$faker = Faker::create();
@@ -180,7 +362,7 @@ public function testCategoryPermission()
180362
// Invalidate permission cache to ensure the new permission takes effect
181363
$this->user->invalidatePermissionCache();
182364

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

186368
// test view permission
@@ -250,8 +432,8 @@ public function testSetPermissionsViewMyRequestForUser()
250432
public function testSetPermissionsViewMyRequestForUsersAndGroupCreated()
251433
{
252434
// Set up the users and groups
253-
$users = User::factory()->count(5)->create();
254-
$groups = Group::factory()->count(3)->create();
435+
User::factory()->count(5)->create();
436+
Group::factory()->count(3)->create();
255437

256438
// Run the seeder
257439
$this->seed(PermissionSeeder::class);
@@ -279,7 +461,7 @@ public function testAdministratorRoleAssignment()
279461
$this->user = $regularUser;
280462
$this->user->save();
281463

282-
$response = $this->apiCall('PUT', '/permissions', [
464+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
283465
'user_id' => $targetUser->id,
284466
'is_administrator' => true,
285467
'permission_names' => [],
@@ -292,7 +474,7 @@ public function testAdministratorRoleAssignment()
292474
$targetUser->is_administrator = true;
293475
$targetUser->save();
294476

295-
$response = $this->apiCall('PUT', '/permissions', [
477+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
296478
'user_id' => $targetUser->id,
297479
'is_administrator' => false,
298480
'permission_names' => [],
@@ -306,7 +488,7 @@ public function testAdministratorRoleAssignment()
306488
$this->user = $adminUser;
307489
$this->user->save();
308490

309-
$response = $this->apiCall('PUT', '/permissions', [
491+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
310492
'user_id' => $targetUser->id,
311493
'is_administrator' => false,
312494
'permission_names' => [],
@@ -317,7 +499,7 @@ public function testAdministratorRoleAssignment()
317499
$this->assertFalse($targetUser->is_administrator);
318500

319501
// Test 4: Admin can grant admin role
320-
$response = $this->apiCall('PUT', '/permissions', [
502+
$response = $this->apiCall('PUT', self::PERMISSIONS_URL, [
321503
'user_id' => $targetUser->id,
322504
'is_administrator' => true,
323505
'permission_names' => [],

0 commit comments

Comments
 (0)