From a7ccf0effffa9d85d6e3771b7b4d58bf6defa50f Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 21 May 2024 09:56:51 +0100 Subject: [PATCH 01/11] Added translation Signed-off-by: snipe --- resources/views/groups/view.blade.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resources/views/groups/view.blade.php b/resources/views/groups/view.blade.php index 9b6c098638..a408266783 100644 --- a/resources/views/groups/view.blade.php +++ b/resources/views/groups/view.blade.php @@ -52,7 +52,7 @@ @if (is_array($group->decodePermissions())) From e3561ad38ebf9032ea75067b2d3881ec2075fb43 Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 21 May 2024 09:56:57 +0100 Subject: [PATCH 02/11] Added tests Signed-off-by: snipe --- tests/Feature/Api/Users/UpdateUserApiTest.php | 72 ++++++++++++++++--- 1 file changed, 63 insertions(+), 9 deletions(-) diff --git a/tests/Feature/Api/Users/UpdateUserApiTest.php b/tests/Feature/Api/Users/UpdateUserApiTest.php index 1728de82cd..16553c2b4d 100644 --- a/tests/Feature/Api/Users/UpdateUserApiTest.php +++ b/tests/Feature/Api/Users/UpdateUserApiTest.php @@ -127,27 +127,27 @@ class UpdateUserApiTest extends TestCase { $groupToJoin = Group::factory()->create(); - $normalUser = User::factory()->editUsers()->create(); + $userWhoCanEditUsers = User::factory()->editUsers()->create(); $superUser = User::factory()->superuser()->create(); - $oneUserToUpdate = User::factory()->create(); - $anotherUserToUpdate = User::factory()->create(); + $userToUpdateByToUpdateuserWhoCanEditUsers = User::factory()->create(); + $userToUpdateByToUpdateuserBySuperuser = User::factory()->create(); - $this->actingAsForApi($normalUser) - ->patchJson(route('api.users.update', $oneUserToUpdate), [ + $this->actingAsForApi($userWhoCanEditUsers) + ->patchJson(route('api.users.update', $userToUpdateByToUpdateuserWhoCanEditUsers), [ 'groups' => [$groupToJoin->id], ]); $this->actingAsForApi($superUser) - ->patchJson(route('api.users.update', $anotherUserToUpdate), [ + ->patchJson(route('api.users.update', $userToUpdateByToUpdateuserBySuperuser), [ 'groups' => [$groupToJoin->id], ]); - $this->assertFalse( - $oneUserToUpdate->refresh()->groups->contains($groupToJoin), + $this->assertFalse($userToUpdateByToUpdateuserWhoCanEditUsers->refresh()->groups->contains($groupToJoin), 'Non-super-user was able to modify user group' ); - $this->assertTrue($anotherUserToUpdate->refresh()->groups->contains($groupToJoin)); + + $this->assertTrue($userToUpdateByToUpdateuserBySuperuser->refresh()->groups->contains($groupToJoin)); } public function testUserGroupsCanBeClearedBySuperUser() @@ -175,4 +175,58 @@ class UpdateUserApiTest extends TestCase $this->assertTrue($oneUserToUpdate->refresh()->groups->contains($joinedGroup)); $this->assertFalse($anotherUserToUpdate->refresh()->groups->contains($joinedGroup)); } + + public function testNonSuperuserCannotUpdateOwnGroups() + { + $groupToJoin = Group::factory()->create(); + + $user = User::factory()->editUsers()->create(); + + $this->actingAsForApi($user) + ->patchJson(route('api.users.update', $user), [ + 'groups' => [$groupToJoin->id], + ]); + + + $this->assertFalse($user->refresh()->groups->contains($groupToJoin), + 'Non-super-user was able to modify user group' + ); + + } + + public function testNonSuperuserCannotUpdateGroups() + { + $user = User::factory()->editUsers()->create(); + $group = Group::factory()->create(); + $user->groups()->sync([$group->id]); + $newGroupToJoin = Group::factory()->create(); + + $this->actingAsForApi($user) + ->patchJson(route('api.users.update', $user), [ + 'groups' => [$newGroupToJoin->id], + ]); + + + $this->assertFalse($user->refresh()->groups->contains($newGroupToJoin), + 'Non-super-user was able to modify user group membership' + ); + + $this->assertTrue($user->refresh()->groups->contains($group)); + + } + + public function testUsersGroupsAreNotClearedIfNoGroupPassedBySuperUser() + { + $user = User::factory()->create(); + $superUser = User::factory()->superuser()->create(); + + $group = Group::factory()->create(); + $user->groups()->sync([$group->id]); + + $this->actingAsForApi($superUser) + ->patchJson(route('api.users.update', $user), []); + + $this->assertTrue($user->refresh()->groups->contains($group)); + } + } From 34f1ea1c0ecd403047cd1327569ee391a7201cc1 Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 21 May 2024 10:00:49 +0100 Subject: [PATCH 03/11] Re-order gating and refactor group syncing Signed-off-by: snipe --- app/Http/Controllers/Api/UsersController.php | 26 ++++++-------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 0faa541243..006b6de9a6 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -475,7 +475,7 @@ class UsersController extends Controller if ($request->has('permissions')) { $permissions_array = $request->input('permissions'); - // Strip out the superuser permission if the API user isn't a superadmin + // Strip out the individual superuser permission if the API user isn't a superadmin if (! Auth::user()->isSuperUser()) { unset($permissions_array['superuser']); } @@ -493,32 +493,20 @@ class UsersController extends Controller if ($user->save()) { - // Check if the request has groups passed and has a value - if ($request->filled('groups')) { + // Check if the request has groups passed and has a value, AND that the user us a superuser + if (($request->has('groups')) && (Auth::user()->isSuperUser())) { $validator = Validator::make($request->all(), [ 'groups.*' => 'integer|exists:permission_groups,id', ]); - - if ($validator->fails()){ + + $user->groups()->sync($request->input('groups')); + + if ($validator->fails()) { return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors())); } - // Only save groups if the user is a superuser - if (Auth::user()->isSuperUser()) { - $user->groups()->sync($request->input('groups')); - } - - // The groups field has been passed but it is null, so we should blank it out - } elseif ($request->has('groups')) { - - // Only save groups if the user is a superuser - if (Auth::user()->isSuperUser()) { - $user->groups()->sync($request->input('groups')); - } } - - return response()->json(Helper::formatStandardApiResponse('success', (new UsersTransformer)->transformUser($user), trans('admin/users/message.success.update'))); } From 0891bd747a235c45ccc571196847e0319fce48ed Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 22 May 2024 11:00:31 +0100 Subject: [PATCH 04/11] Update tests/Feature/Api/Users/UpdateUserApiTest.php Co-authored-by: Marcus Moore --- tests/Feature/Api/Users/UpdateUserApiTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Feature/Api/Users/UpdateUserApiTest.php b/tests/Feature/Api/Users/UpdateUserApiTest.php index 16553c2b4d..91a2122c74 100644 --- a/tests/Feature/Api/Users/UpdateUserApiTest.php +++ b/tests/Feature/Api/Users/UpdateUserApiTest.php @@ -130,8 +130,8 @@ class UpdateUserApiTest extends TestCase $userWhoCanEditUsers = User::factory()->editUsers()->create(); $superUser = User::factory()->superuser()->create(); - $userToUpdateByToUpdateuserWhoCanEditUsers = User::factory()->create(); - $userToUpdateByToUpdateuserBySuperuser = User::factory()->create(); + $userToUpdateByUserWhoCanEditUsers = User::factory()->create(); + $userToUpdateByToUserBySuperuser = User::factory()->create(); $this->actingAsForApi($userWhoCanEditUsers) ->patchJson(route('api.users.update', $userToUpdateByToUpdateuserWhoCanEditUsers), [ From 3afe996755ec744bd0b5677d82ecf44e636bc126 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 22 May 2024 11:00:47 +0100 Subject: [PATCH 05/11] Update tests/Feature/Api/Users/UpdateUserApiTest.php Co-authored-by: Marcus Moore --- tests/Feature/Api/Users/UpdateUserApiTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Feature/Api/Users/UpdateUserApiTest.php b/tests/Feature/Api/Users/UpdateUserApiTest.php index 91a2122c74..ecb932300c 100644 --- a/tests/Feature/Api/Users/UpdateUserApiTest.php +++ b/tests/Feature/Api/Users/UpdateUserApiTest.php @@ -134,7 +134,7 @@ class UpdateUserApiTest extends TestCase $userToUpdateByToUserBySuperuser = User::factory()->create(); $this->actingAsForApi($userWhoCanEditUsers) - ->patchJson(route('api.users.update', $userToUpdateByToUpdateuserWhoCanEditUsers), [ + ->patchJson(route('api.users.update', $userToUpdateByUserWhoCanEditUsers), [ 'groups' => [$groupToJoin->id], ]); From 14c4765be205d84975203eca550484b4d4803876 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 22 May 2024 11:00:52 +0100 Subject: [PATCH 06/11] Update tests/Feature/Api/Users/UpdateUserApiTest.php Co-authored-by: Marcus Moore --- tests/Feature/Api/Users/UpdateUserApiTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Feature/Api/Users/UpdateUserApiTest.php b/tests/Feature/Api/Users/UpdateUserApiTest.php index ecb932300c..3790a7acc9 100644 --- a/tests/Feature/Api/Users/UpdateUserApiTest.php +++ b/tests/Feature/Api/Users/UpdateUserApiTest.php @@ -143,7 +143,7 @@ class UpdateUserApiTest extends TestCase 'groups' => [$groupToJoin->id], ]); - $this->assertFalse($userToUpdateByToUpdateuserWhoCanEditUsers->refresh()->groups->contains($groupToJoin), + $this->assertFalse($userToUpdateByUserWhoCanEditUsers->refresh()->groups->contains($groupToJoin), 'Non-super-user was able to modify user group' ); From 51025aad5735c837b8bf21e191adcad794edba23 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 22 May 2024 11:03:38 +0100 Subject: [PATCH 07/11] Update tests/Feature/Api/Users/UpdateUserApiTest.php Co-authored-by: Marcus Moore --- tests/Feature/Api/Users/UpdateUserApiTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Feature/Api/Users/UpdateUserApiTest.php b/tests/Feature/Api/Users/UpdateUserApiTest.php index 3790a7acc9..40815e4d3d 100644 --- a/tests/Feature/Api/Users/UpdateUserApiTest.php +++ b/tests/Feature/Api/Users/UpdateUserApiTest.php @@ -139,7 +139,7 @@ class UpdateUserApiTest extends TestCase ]); $this->actingAsForApi($superUser) - ->patchJson(route('api.users.update', $userToUpdateByToUpdateuserBySuperuser), [ + ->patchJson(route('api.users.update', $userToUpdateByToUserBySuperuser), [ 'groups' => [$groupToJoin->id], ]); From e1eb457f3dc4cfb88222ed059a1654999a2bade5 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 22 May 2024 11:03:46 +0100 Subject: [PATCH 08/11] Update tests/Feature/Api/Users/UpdateUserApiTest.php Co-authored-by: Marcus Moore --- tests/Feature/Api/Users/UpdateUserApiTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Feature/Api/Users/UpdateUserApiTest.php b/tests/Feature/Api/Users/UpdateUserApiTest.php index 40815e4d3d..f5bd5d1e83 100644 --- a/tests/Feature/Api/Users/UpdateUserApiTest.php +++ b/tests/Feature/Api/Users/UpdateUserApiTest.php @@ -147,7 +147,7 @@ class UpdateUserApiTest extends TestCase 'Non-super-user was able to modify user group' ); - $this->assertTrue($userToUpdateByToUpdateuserBySuperuser->refresh()->groups->contains($groupToJoin)); + $this->assertTrue($userToUpdateByToUserBySuperuser->refresh()->groups->contains($groupToJoin)); } public function testUserGroupsCanBeClearedBySuperUser() From bb96a190fdb8618ae359d8f8e428130a75879000 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 22 May 2024 12:34:48 +0100 Subject: [PATCH 09/11] Moved validator Signed-off-by: snipe --- app/Http/Controllers/Api/UsersController.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 006b6de9a6..24def9414e 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -496,17 +496,20 @@ class UsersController extends Controller // Check if the request has groups passed and has a value, AND that the user us a superuser if (($request->has('groups')) && (Auth::user()->isSuperUser())) { - $validator = Validator::make($request->all(), [ + $validator = Validator::make($request->only('groups'), [ 'groups.*' => 'integer|exists:permission_groups,id', ]); - $user->groups()->sync($request->input('groups')); - if ($validator->fails()) { - return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors())); + return response()->json(Helper::formatStandardApiResponse('error', null, $validator->errors())); } + // Sync the groups since the user is a superuser and the groups pass validation + $user->groups()->sync($request->input('groups')); + + } + return response()->json(Helper::formatStandardApiResponse('success', (new UsersTransformer)->transformUser($user), trans('admin/users/message.success.update'))); } From 4b23ef402143778bad65ef7ef6a6095df58b8e6f Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 22 May 2024 12:35:03 +0100 Subject: [PATCH 10/11] Added new test Signed-off-by: snipe --- tests/Feature/Api/Users/UpdateUserApiTest.php | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/tests/Feature/Api/Users/UpdateUserApiTest.php b/tests/Feature/Api/Users/UpdateUserApiTest.php index f5bd5d1e83..af5d42e64b 100644 --- a/tests/Feature/Api/Users/UpdateUserApiTest.php +++ b/tests/Feature/Api/Users/UpdateUserApiTest.php @@ -179,7 +179,6 @@ class UpdateUserApiTest extends TestCase public function testNonSuperuserCannotUpdateOwnGroups() { $groupToJoin = Group::factory()->create(); - $user = User::factory()->editUsers()->create(); $this->actingAsForApi($user) @@ -187,7 +186,6 @@ class UpdateUserApiTest extends TestCase 'groups' => [$groupToJoin->id], ]); - $this->assertFalse($user->refresh()->groups->contains($groupToJoin), 'Non-super-user was able to modify user group' ); @@ -229,4 +227,22 @@ class UpdateUserApiTest extends TestCase $this->assertTrue($user->refresh()->groups->contains($group)); } + public function testMultipleGroupsUpdateBySuperUser() + { + $user = User::factory()->create(); + $superUser = User::factory()->superuser()->create(); + + $groupA = Group::factory()->create(['name'=>'Group A']); + $groupB = Group::factory()->create(['name'=>'Group B']); + + $response = $this->actingAsForApi($superUser) + ->patchJson(route('api.users.update', $user), [ + 'groups' => [$groupA->id, $groupB->id], + ])->json(); + + \Log::error($response); + $this->assertTrue($user->refresh()->groups->contains($groupA)); + $this->assertTrue($user->refresh()->groups->contains($groupB)); + } + } From 6fad085a7d1e6ea28ba0fd6f5c05460210fa379a Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 22 May 2024 12:37:49 +0100 Subject: [PATCH 11/11] Removed debug in test Signed-off-by: snipe --- tests/Feature/Api/Users/UpdateUserApiTest.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/Feature/Api/Users/UpdateUserApiTest.php b/tests/Feature/Api/Users/UpdateUserApiTest.php index af5d42e64b..37ebce4a46 100644 --- a/tests/Feature/Api/Users/UpdateUserApiTest.php +++ b/tests/Feature/Api/Users/UpdateUserApiTest.php @@ -235,12 +235,11 @@ class UpdateUserApiTest extends TestCase $groupA = Group::factory()->create(['name'=>'Group A']); $groupB = Group::factory()->create(['name'=>'Group B']); - $response = $this->actingAsForApi($superUser) + $this->actingAsForApi($superUser) ->patchJson(route('api.users.update', $user), [ 'groups' => [$groupA->id, $groupB->id], ])->json(); - \Log::error($response); $this->assertTrue($user->refresh()->groups->contains($groupA)); $this->assertTrue($user->refresh()->groups->contains($groupB)); }