diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 9200f80b1d..856b3b6a69 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -427,13 +427,10 @@ class UsersController extends Controller * @param \Illuminate\Http\Request $request * @param int $id */ - public function update(SaveUserRequest $request, $id) : JsonResponse + public function update(SaveUserRequest $request, User $user): JsonResponse { $this->authorize('update', User::class); - if ($user = User::find($id)) { - - $this->authorize('update', $user); /** @@ -443,12 +440,10 @@ class UsersController extends Controller * */ - - if ((($id == 1) || ($id == 2)) && (config('app.lock_passwords'))) { + if ((($user->id == 1) || ($user->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->fill($request->all()); if ($user->id == $request->input('manager_id')) { @@ -473,16 +468,13 @@ class UsersController extends Controller $user->permissions = $permissions_array; } - // 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())) { @@ -496,18 +488,10 @@ class UsersController extends Controller // 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, trans('admin/users/message.user_not_found', compact('id')))); - } /** diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index 1965962adc..1d7fc91ebd 100755 --- a/app/Http/Controllers/Users/UsersController.php +++ b/app/Http/Controllers/Users/UsersController.php @@ -186,7 +186,7 @@ class UsersController extends Controller { $this->authorize('update', User::class); - $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed()->find($id); + $user = User::with(['assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc'])->withTrashed()->find($id); if ($user) { @@ -214,83 +214,79 @@ class UsersController extends Controller * @return \Illuminate\Http\RedirectResponse * @throws \Illuminate\Auth\Access\AuthorizationException */ - public function update(SaveUserRequest $request, $id = null) + public function update(SaveUserRequest $request, User $user) { $this->authorize('update', User::class); // 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 ((($id == 1) || ($id == 2)) && (config('app.lock_passwords'))) { + if ((($user->id == 1) || ($user->id == 2)) && (config('app.lock_passwords'))) { return redirect()->route('users.index')->with('error', trans('general.permission_denied_superuser_demo')); } - // We need to reverse the UI specific logic for our // permissions here before we update the user. $permissions = $request->input('permissions', []); app('request')->request->set('permissions', $permissions); - $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed()->find($id); + $user->load(['assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc'])->withTrashed(); - // User is valid - continue... - if ($user) { - $this->authorize('update', $user); + $this->authorize('update', $user); - // Figure out of this user was an admin before this edit - $orig_permissions_array = $user->decodePermissions(); - $orig_superuser = '0'; - if (is_array($orig_permissions_array)) { - if (array_key_exists('superuser', $orig_permissions_array)) { - $orig_superuser = $orig_permissions_array['superuser']; - } + // Figure out of this user was an admin before this edit + $orig_permissions_array = $user->decodePermissions(); + $orig_superuser = '0'; + if (is_array($orig_permissions_array)) { + if (array_key_exists('superuser', $orig_permissions_array)) { + $orig_superuser = $orig_permissions_array['superuser']; } + } - // Only save groups if the user is a superuser - if (auth()->user()->isSuperUser()) { - $user->groups()->sync($request->input('groups')); - } + // Only save groups if the user is a superuser + if (auth()->user()->isSuperUser()) { + $user->groups()->sync($request->input('groups')); + } - // Update the user fields - $user->username = trim($request->input('username')); - $user->email = trim($request->input('email')); - $user->first_name = $request->input('first_name'); - $user->last_name = $request->input('last_name'); - $user->two_factor_optin = $request->input('two_factor_optin') ?: 0; - $user->locale = $request->input('locale'); - $user->employee_num = $request->input('employee_num'); - $user->activated = $request->input('activated', 0); - $user->jobtitle = $request->input('jobtitle', null); - $user->phone = $request->input('phone'); - $user->location_id = $request->input('location_id', null); - $user->company_id = Company::getIdForUser($request->input('company_id', null)); - $user->manager_id = $request->input('manager_id', null); - $user->notes = $request->input('notes'); - $user->department_id = $request->input('department_id', null); - $user->address = $request->input('address', null); - $user->city = $request->input('city', null); - $user->state = $request->input('state', null); - $user->country = $request->input('country', null); - // if a user is editing themselves we should always keep activated true - $user->activated = $request->input('activated', $request->user()->is($user) ? 1 : 0); - $user->zip = $request->input('zip', null); - $user->remote = $request->input('remote', 0); - $user->vip = $request->input('vip', 0); - $user->website = $request->input('website', null); - $user->start_date = $request->input('start_date', null); - $user->end_date = $request->input('end_date', null); - $user->autoassign_licenses = $request->input('autoassign_licenses', 0); + // Update the user fields + $user->username = trim($request->input('username')); + $user->email = trim($request->input('email')); + $user->first_name = $request->input('first_name'); + $user->last_name = $request->input('last_name'); + $user->two_factor_optin = $request->input('two_factor_optin') ?: 0; + $user->locale = $request->input('locale'); + $user->employee_num = $request->input('employee_num'); + $user->activated = $request->input('activated', 0); + $user->jobtitle = $request->input('jobtitle', null); + $user->phone = $request->input('phone'); + $user->location_id = $request->input('location_id', null); + $user->company_id = Company::getIdForUser($request->input('company_id', null)); + $user->manager_id = $request->input('manager_id', null); + $user->notes = $request->input('notes'); + $user->department_id = $request->input('department_id', null); + $user->address = $request->input('address', null); + $user->city = $request->input('city', null); + $user->state = $request->input('state', null); + $user->country = $request->input('country', null); + // if a user is editing themselves we should always keep activated true + $user->activated = $request->input('activated', $request->user()->is($user) ? 1 : 0); + $user->zip = $request->input('zip', null); + $user->remote = $request->input('remote', 0); + $user->vip = $request->input('vip', 0); + $user->website = $request->input('website', null); + $user->start_date = $request->input('start_date', null); + $user->end_date = $request->input('end_date', null); + $user->autoassign_licenses = $request->input('autoassign_licenses', 0); - // 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)]); + // 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)]); - // Do we want to update the user password? - if ($request->filled('password')) { - $user->password = bcrypt($request->input('password')); - } + // Do we want to update the user password? + if ($request->filled('password')) { + $user->password = bcrypt($request->input('password')); + } // Update the location of any assets checked out to this user @@ -318,13 +314,7 @@ class UsersController extends Controller return redirect()->to(Helper::getRedirectOption($request, $user->id, 'Users')) ->with('success', trans('admin/users/message.success.update')); } - return redirect()->back()->withInput()->withErrors($user->getErrors()); - - - } - - return redirect()->route('users.index')->with('error', trans('admin/users/message.user_not_found', compact('id'))); } /** diff --git a/app/Http/Requests/SaveUserRequest.php b/app/Http/Requests/SaveUserRequest.php index b38193c15a..e16826d16c 100644 --- a/app/Http/Requests/SaveUserRequest.php +++ b/app/Http/Requests/SaveUserRequest.php @@ -34,6 +34,14 @@ class SaveUserRequest extends FormRequest $rules = [ 'department_id' => 'nullable|exists:departments,id', 'manager_id' => 'nullable|exists:users,id', + 'company_id' => [ + // determines if the user is being moved between companies and checks to see if they have any items assigned + function ($attribute, $value, $fail) { + if (($this->has('company_id')) && ($this->user->allAssignedCount() > 0) && (Setting::getSettings()->full_multiple_companies_support)) { + $fail(trans('admin/users/message.error.multi_company_items_assigned')); + } + } + ] ]; switch ($this->method()) { diff --git a/resources/lang/en-US/admin/users/message.php b/resources/lang/en-US/admin/users/message.php index 4d014775bd..0f41d463e1 100644 --- a/resources/lang/en-US/admin/users/message.php +++ b/resources/lang/en-US/admin/users/message.php @@ -53,6 +53,7 @@ return array( 'ldap_could_not_search' => 'Could not search the LDAP server. Please check your LDAP server configuration in the LDAP config file.
Error from LDAP Server:', 'ldap_could_not_get_entries' => 'Could not get entries from the LDAP server. Please check your LDAP server configuration in the LDAP config file.
Error from LDAP Server:', 'password_ldap' => 'The password for this account is managed by LDAP/Active Directory. Please contact your IT department to change your password. ', + 'multi_company_items_assigned' => 'This user has items assigned, please check them in before moving companies.' ), 'deletefile' => array( diff --git a/resources/views/users/edit.blade.php b/resources/views/users/edit.blade.php index 1c23b76823..9b4080a339 100755 --- a/resources/views/users/edit.blade.php +++ b/resources/views/users/edit.blade.php @@ -66,7 +66,9 @@
-
+ {{csrf_field()}} @if($user->id) diff --git a/routes/api.php b/routes/api.php index 35e6c92060..92fa24b91f 100644 --- a/routes/api.php +++ b/routes/api.php @@ -1097,18 +1097,20 @@ Route::group(['prefix' => 'v1', 'middleware' => ['api', 'throttle:api']], functi ] )->name('api.users.restore'); - }); + }); + + Route::match(['put', 'patch'], '{user}/update', [Api\UsersController::class, 'update']) + ->name('api.users.update'); Route::resource('users', Api\UsersController::class, ['names' => [ 'index' => 'api.users.index', 'show' => 'api.users.show', - 'update' => 'api.users.update', 'store' => 'api.users.store', 'destroy' => 'api.users.destroy', ], - 'except' => ['create', 'edit'], + 'except' => ['create', 'edit', 'update'], 'parameters' => ['user' => 'user_id'], ] ); // end users API routes diff --git a/routes/web/users.php b/routes/web/users.php index 95de200638..e55541a937 100644 --- a/routes/web/users.php +++ b/routes/web/users.php @@ -145,10 +145,12 @@ Route::group(['prefix' => 'users', 'middleware' => ['auth']], function () { ] )->name('users/bulkeditsave'); - + // pulling this out of the resource because I need route model binding in the request + Route::match(['put', 'patch'], '/{user}', [Users\UsersController::class, 'update'])->name('users.update'); }); Route::resource('users', Users\UsersController::class, [ 'middleware' => ['auth'], 'parameters' => ['user' => 'user_id'], + 'except' => ['update'] ]); \ No newline at end of file diff --git a/tests/Feature/Users/Api/UpdateUserTest.php b/tests/Feature/Users/Api/UpdateUserTest.php index 1c66bbdda9..4632a80c3d 100644 --- a/tests/Feature/Users/Api/UpdateUserTest.php +++ b/tests/Feature/Users/Api/UpdateUserTest.php @@ -2,6 +2,7 @@ namespace Tests\Feature\Users\Api; +use App\Models\Asset; use App\Models\Company; use App\Models\Department; use App\Models\Group; @@ -344,4 +345,47 @@ class UpdateUserTest extends TestCase $this->assertTrue($user->refresh()->groups->contains($groupB)); } + public function testMultiCompanyUserCannotBeMovedIfHasAsset() + { + $this->settings->enableMultipleFullCompanySupport(); + + $companyA = Company::factory()->create(); + $companyB = Company::factory()->create(); + + $user = User::factory()->create([ + 'company_id' => $companyA->id, + ]); + $superUser = User::factory()->superuser()->create(); + + $asset = Asset::factory()->create(); + + // no assets assigned, therefore success + $this->actingAsForApi($superUser)->patchJson(route('api.users.update', $user), [ + 'username' => 'test', + 'company_id' => $companyB->id, + ])->assertStatusMessageIs('success'); + + // same test but PUT + $this->actingAsForApi($superUser)->putJson(route('api.users.update', $user), [ + 'username' => 'test', + 'first_name' => 'Test', + 'company_id' => $companyB->id, + ])->assertStatusMessageIs('success'); + + $asset->checkOut($user, $superUser); + + // asset assigned, therefore error + $this->actingAsForApi($superUser)->patchJson(route('api.users.update', $user), [ + 'username' => 'test', + 'company_id' => $companyB->id, + ])->assertStatusMessageIs('error'); + + // same test but PUT + $this->actingAsForApi($superUser)->putJson(route('api.users.update', $user), [ + 'username' => 'test', + 'first_name' => 'Test', + 'company_id' => $companyB->id, + ])->assertStatusMessageIs('error'); + } + } diff --git a/tests/Feature/Users/Ui/UpdateUserTest.php b/tests/Feature/Users/Ui/UpdateUserTest.php index bef1d59a06..3e6867021f 100644 --- a/tests/Feature/Users/Ui/UpdateUserTest.php +++ b/tests/Feature/Users/Ui/UpdateUserTest.php @@ -2,6 +2,8 @@ namespace Tests\Feature\Users\Ui; +use App\Models\Asset; +use App\Models\Company; use App\Models\User; use Tests\TestCase; @@ -79,4 +81,39 @@ class UpdateUserTest extends TestCase $this->assertEquals(1, $admin->refresh()->activated); } + + public function testMultiCompanyUserCannotBeMovedIfHasAsset() + { + $this->settings->enableMultipleFullCompanySupport(); + + $companyA = Company::factory()->create(); + $companyB = Company::factory()->create(); + + $user = User::factory()->create([ + 'company_id' => $companyA->id, + ]); + $superUser = User::factory()->superuser()->create(); + + $asset = Asset::factory()->create(); + + // no assets assigned, therefore success + $this->actingAs($superUser)->put(route('users.update', $user), [ + 'first_name' => 'test', + 'username' => 'test', + 'company_id' => $companyB->id, + 'redirect_option' => 'index' + ])->assertRedirect(route('users.index')); + + $asset->checkOut($user, $superUser); + + // asset assigned, therefore error + $response = $this->actingAs($superUser)->patchJson(route('users.update', $user), [ + 'first_name' => 'test', + 'username' => 'test', + 'company_id' => $companyB->id, + 'redirect_option' => 'index' + ]); + + $this->followRedirects($response)->assertSee('error'); + } }