Merge remote-tracking branch 'origin/develop'

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
+            // Check if the request has groups passed and has a value, AND that the user us a superuser
-            if ($request->filled('groups')) {
+            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()){
+                if ($validator->fails()) {
-                    return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors()));
+                    return response()->json(Helper::formatStandardApiResponse('error', null, $validator->errors()));
                 }
 
-                // Only save groups if the user is a superuser
+                // Sync the groups since the user is a superuser and the groups pass validation
-                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'));
-                }
             }
 
-
             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 @@
             @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>

resources/views/settings/index.blade.php

@@ -446,8 +446,8 @@
         </div>
       </div> <!-- /box-body-->
     </div> <!--/box-default-->
-  </div><!--/col-md-8-->
+
-</div><!--/row-->
+
 
 
 

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();
+        $userToUpdateByUserWhoCanEditUsers = User::factory()->create();
-        $anotherUserToUpdate = User::factory()->create();
+        $userToUpdateByToUserBySuperuser = User::factory()->create();
 
-        $this->actingAsForApi($normalUser)
+        $this->actingAsForApi($userWhoCanEditUsers)
-            ->patchJson(route('api.users.update', $oneUserToUpdate), [
+            ->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(
+        $this->assertFalse($userToUpdateByUserWhoCanEditUsers->refresh()->groups->contains($groupToJoin),
-            $oneUserToUpdate->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));
+    }
+
 }