Refactor group syncing on user edit API endpoint

app/Http/Controllers/Api/UsersController.php

@@ -475,7 +475,7 @@ public function update(SaveUserRequest $request, $id)
         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 @@ public function update(SaveUserRequest $request, $id)
 
         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()){
-                    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'));
-                }
+                $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'));
+                if ($validator->fails()) {
+                    return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors()));
                 }
-            }
-
 
+            }
             return response()->json(Helper::formatStandardApiResponse('success', (new UsersTransformer)->transformUser($user), trans('admin/users/message.success.update')));
         }
 

resources/views/groups/view.blade.php

@@ -52,7 +52,7 @@ class="table table-striped snipe-table"
             @if (is_array($group->decodePermissions()))
             <ul class="list-unstyled">
                 @foreach ($group->decodePermissions() as $permission_name => $permission)
-                   <li>{!! ($permission == '1') ? '<i class="fas fa-check text-success" aria-hidden="true"></i><span class="sr-only">GRANTED: </span>' :  '<i class="fas fa-times text-danger" aria-hidden="true"></i><span class="sr-only">DENIED: </span>' !!} {{ e(str_replace('.', ': ', ucwords($permission_name))) }} </li>
+                   <li>{!! ($permission == '1') ? '<i class="fas fa-check text-success" aria-hidden="true"></i><span class="sr-only">'.trans('general.yes').': </span>' :  '<i class="fas fa-times text-danger" aria-hidden="true"></i><span class="sr-only">'.trans('general.no').': </span>' !!} {{ e(str_replace('.', ': ', ucwords($permission_name))) }} </li>
                 @endforeach
 
             </ul>

tests/Feature/Api/Users/UpdateUserApiTest.php

@@ -127,27 +127,27 @@ public function testUserGroupsAreOnlyUpdatedIfAuthenticatedUserIsSuperUser()
     {
         $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,58 @@ public function testUserGroupsCanBeClearedBySuperUser()
         $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));
+    }
+
 }