diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 0faa541243..24def9414e 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,23 @@ 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(), [ + $validator = Validator::make($request->only('groups'), [ 'groups.*' => 'integer|exists:permission_groups,id', ]); - - if ($validator->fails()){ - return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors())); + + if ($validator->fails()) { + return response()->json(Helper::formatStandardApiResponse('error', null, $validator->errors())); } - // Only save groups if the user is a superuser - if (Auth::user()->isSuperUser()) { - $user->groups()->sync($request->input('groups')); - } + // Sync the groups since the user is a superuser and the groups pass validation + $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'))); } 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())) diff --git a/tests/Feature/Api/Users/UpdateUserApiTest.php b/tests/Feature/Api/Users/UpdateUserApiTest.php index 1728de82cd..37ebce4a46 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(); + $userToUpdateByUserWhoCanEditUsers = User::factory()->create(); + $userToUpdateByToUserBySuperuser = User::factory()->create(); - $this->actingAsForApi($normalUser) - ->patchJson(route('api.users.update', $oneUserToUpdate), [ + $this->actingAsForApi($userWhoCanEditUsers) + ->patchJson(route('api.users.update', $userToUpdateByUserWhoCanEditUsers), [ 'groups' => [$groupToJoin->id], ]); $this->actingAsForApi($superUser) - ->patchJson(route('api.users.update', $anotherUserToUpdate), [ + ->patchJson(route('api.users.update', $userToUpdateByToUserBySuperuser), [ 'groups' => [$groupToJoin->id], ]); - $this->assertFalse( - $oneUserToUpdate->refresh()->groups->contains($groupToJoin), + $this->assertFalse($userToUpdateByUserWhoCanEditUsers->refresh()->groups->contains($groupToJoin), 'Non-super-user was able to modify user group' ); - $this->assertTrue($anotherUserToUpdate->refresh()->groups->contains($groupToJoin)); + + $this->assertTrue($userToUpdateByToUserBySuperuser->refresh()->groups->contains($groupToJoin)); } public function testUserGroupsCanBeClearedBySuperUser() @@ -175,4 +175,73 @@ 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)); + } + + 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']); + + $this->actingAsForApi($superUser) + ->patchJson(route('api.users.update', $user), [ + 'groups' => [$groupA->id, $groupB->id], + ])->json(); + + $this->assertTrue($user->refresh()->groups->contains($groupA)); + $this->assertTrue($user->refresh()->groups->contains($groupB)); + } + }