Merge pull request #14982 from snipe/fixes/check_for_user_on_patch_api

Check that the user exists before trying to fill the request

app/Http/Controllers/Api/UsersController.php

@@ -437,77 +437,83 @@ class UsersController extends Controller
     {
         $this->authorize('update', User::class);
 
-        $user = User::find($id);
-        $this->authorize('update', $user);
-
-        /**
-         * This is a janky hack to prevent people from changing admin demo user data on the public demo.
-         * The $ids 1 and 2 are special since they are seeded as superadmins in the demo seeder.
-         *  Thanks, jerks. You are why we can't have nice things. - snipe
-         * 
-         */ 
+        if ($user = User::find($id)) {
 
 
-        if ((($id == 1) || ($id == 2)) && (config('app.lock_passwords'))) {
-            return response()->json(Helper::formatStandardApiResponse('error', null, 'Permission denied. You cannot update user information via API on the demo.'));
-        }
+            $this->authorize('update', $user);
+
+            /**
+             * This is a janky hack to prevent people from changing admin demo user data on the public demo.
+             * The $ids 1 and 2 are special since they are seeded as superadmins in the demo seeder.
+             *  Thanks, jerks. You are why we can't have nice things. - snipe
+             *
+             */
 
 
-        $user->fill($request->all());
-        
-        if ($user->id == $request->input('manager_id')) {
-            return response()->json(Helper::formatStandardApiResponse('error', null, 'You cannot be your own manager'));
-        }
-
-        if ($request->filled('password')) {
-            $user->password = bcrypt($request->input('password'));
-        }
-
-        // We need to use has()  instead of filled()
-        // here because we need to overwrite permissions
-        // if someone needs to null them out
-        if ($request->has('permissions')) {
-            $permissions_array = $request->input('permissions');
-
-            // Strip out the individual superuser permission if the API user isn't a superadmin
-            if (! Auth::user()->isSuperUser()) {
-                unset($permissions_array['superuser']);
+            if ((($id == 1) || ($id == 2)) && (config('app.lock_passwords'))) {
+                return response()->json(Helper::formatStandardApiResponse('error', null, 'Permission denied. You cannot update user information via API on the demo.'));
             }
 
-            $user->permissions = $permissions_array;
-        }
 
+            $user->fill($request->all());
 
-        // Update the location of any assets checked out to this user
-        Asset::where('assigned_type', User::class)
-            ->where('assigned_to', $user->id)->update(['location_id' => $request->input('location_id', null)]);
+            if ($user->id == $request->input('manager_id')) {
+                return response()->json(Helper::formatStandardApiResponse('error', null, 'You cannot be your own manager'));
+            }
 
-        
-        app('App\Http\Requests\ImageUploadRequest')->handleImages($user, 600, 'image', 'avatars', 'avatar');
-          
-        if ($user->save()) {
+            if ($request->filled('password')) {
+                $user->password = bcrypt($request->input('password'));
+            }
 
-            // 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())) {
+            // We need to use has()  instead of filled()
+            // here because we need to overwrite permissions
+            // if someone needs to null them out
+            if ($request->has('permissions')) {
+                $permissions_array = $request->input('permissions');
 
-                $validator = Validator::make($request->only('groups'), [
-                    'groups.*' => 'integer|exists:permission_groups,id',
-                ]);
-
-                if ($validator->fails()) {
-                    return response()->json(Helper::formatStandardApiResponse('error', null, $validator->errors()));
+                // Strip out the individual superuser permission if the API user isn't a superadmin
+                if (!Auth::user()->isSuperUser()) {
+                    unset($permissions_array['superuser']);
                 }
 
-                // Sync the groups since the user is a superuser and the groups pass validation
-                $user->groups()->sync($request->input('groups'));
-
-
+                $user->permissions = $permissions_array;
             }
 
-            return response()->json(Helper::formatStandardApiResponse('success', (new UsersTransformer)->transformUser($user), trans('admin/users/message.success.update')));
+
+            // Update the location of any assets checked out to this user
+            Asset::where('assigned_type', User::class)
+                ->where('assigned_to', $user->id)->update(['location_id' => $request->input('location_id', null)]);
+
+
+            app('App\Http\Requests\ImageUploadRequest')->handleImages($user, 600, 'image', 'avatars', 'avatar');
+
+            if ($user->save()) {
+
+                // 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->only('groups'), [
+                        'groups.*' => 'integer|exists:permission_groups,id',
+                    ]);
+
+                    if ($validator->fails()) {
+                        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')));
+            }
+
+            return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors()));
         }
 
-        return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors()));
+        return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.user_not_found', compact('id'))));
+
     }
 
     /**

tests/Feature/Users/Api/UpdateUserTest.php

@@ -153,47 +153,74 @@ class UpdateUserTest extends TestCase
         // Admin for Company A should allow updating user from Company A
         $this->actingAsForApi($adminA)
             ->patchJson(route('api.users.update', $scoped_user_in_companyA))
-            ->assertStatus(200);
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('success')
+            ->json();
 
         // Admin for Company A should get denied updating user from Company B
         $this->actingAsForApi($adminA)
             ->patchJson(route('api.users.update', $scoped_user_in_companyB))
-            ->assertStatus(403);
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('error')
+            ->json();
 
         // Admin for Company A should get denied updating user without a company
         $this->actingAsForApi($adminA)
             ->patchJson(route('api.users.update', $scoped_user_in_no_company))
-            ->assertStatus(403);
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('error')
+            ->json();
 
         // Admin for Company B should allow updating user from Company B
         $this->actingAsForApi($adminB)
             ->patchJson(route('api.users.update', $scoped_user_in_companyB))
-            ->assertStatus(200);
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('success')
+            ->json();
 
         // Admin for Company B should get denied updating user from Company A
         $this->actingAsForApi($adminB)
             ->patchJson(route('api.users.update', $scoped_user_in_companyA))
-            ->assertStatus(403);
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('error')
+            ->json();
 
         // Admin for Company B should get denied updating user without a company
         $this->actingAsForApi($adminB)
             ->patchJson(route('api.users.update', $scoped_user_in_no_company))
-            ->assertStatus(403);
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('error')
+            ->json();
 
         // Admin without a company should allow updating user without a company
         $this->actingAsForApi($adminNoCompany)
             ->patchJson(route('api.users.update', $scoped_user_in_no_company))
-            ->assertStatus(200);
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('success')
+            ->json();
 
         // Admin without a company should get denied updating user from Company A
         $this->actingAsForApi($adminNoCompany)
             ->patchJson(route('api.users.update', $scoped_user_in_companyA))
-            ->assertStatus(403);
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('error')
+            ->json();
 
         // Admin without a company should get denied updating user from Company B
         $this->actingAsForApi($adminNoCompany)
             ->patchJson(route('api.users.update', $scoped_user_in_companyB))
-            ->assertStatus(403);
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('error')
+            ->json();
     }
 
     public function testUserGroupsAreOnlyUpdatedIfAuthenticatedUserIsSuperUser()